Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core/select): check if value is defined, before updating selection #1557

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

matthiashader
Copy link
Collaborator

@matthiashader matthiashader commented Nov 11, 2024

Addition to #1548

💡 What is the current behavior?

  • select in angular logs an error to the console when initializing the select-items in async
  • last element has always an undefined value
  • angular seems to initialize components differently than react (possibly due to the vdom)

GitHub Issue Number: N/A, [IX-1918]

🆕 What is the new behavior?

  • before the selection is updated, it is checked if the value is defined
  • valueChange emited and selection updated
  • updateSelection still needed when select properties change!
  • removed "get dropdownItem()", method is never called - only appearance in select.tsx does not call this methods
  • only needed for the addItem, which is not a select-item (it is a dropdown-item)
  • label / value changes only occur after the components has been initalised -> fewer events are thrown, also the slotChange takes care about added / deleted slots

🏁 Checklist

A pull request can only be merged if all of these conditions are met (where applicable):

  • 🦮 Accessibility (a11y) features were implemented
  • 🗺️ Internationalization (i18n) - no hard coded strings
  • 📲 Responsiveness - components handle viewport changes and content overflow gracefully
  • 📄 Documentation was reviewed/updated (pnpm run docs)
  • 🧪 Unit tests were added/updated and pass (pnpm test)
  • 📸 Visual regression tests were added/updated and pass (Guide)
  • 🧐 Static code analysis passes (pnpm lint)
  • 🏗️ Successful compilation (pnpm build, changes pushed)

👨‍💻 Help & support

Copy link

changeset-bot bot commented Nov 11, 2024

🦋 Changeset detected

Latest commit: 27c39bc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@siemens/ix Patch
@siemens/ix-angular Patch
@siemens/ix-react Patch
@siemens/ix-vue Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matthiashader matthiashader marked this pull request as draft November 11, 2024 09:01
Copy link
Contributor

github-actions bot commented Nov 11, 2024

Report of strict check

src/components/select/select.tsx
  • src/components/select/select.tsx(336,5): error TS2322: Type '(string | undefined)[]' is not assignable to type 'string[]'.: error TS2322: Type '(string | undefined)[]' is not assignable to type 'string[]'.
  • src/components/select/select.tsx(437,24): error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.: error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  • src/components/select/select.tsx(575,12): error TS18048: 'item.label' is possibly 'undefined'.: error TS18048: 'item.label' is possibly 'undefined'.

@matthiashader matthiashader marked this pull request as ready for review November 14, 2024 11:29
packages/core/src/components/select-item/select-item.tsx Outdated Show resolved Hide resolved
Comment on lines 379 to 388
disconnectedCallback() {
if (this.itemMutationObserver) {
this.itemMutationObserver.disconnect();
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Collaborator Author

@matthiashader matthiashader Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, isn't neccessary with the fixed typings through the memory leak pr.

Copy link

sonarcloud bot commented Nov 15, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants