feat(core/ix-select): Add +1 chip#2371
Conversation
🦋 Changeset detectedLatest commit: 36c9f3e The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
Summary of ChangesHello @lzeiml, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to handle overflow of selected items in a multi-select by displaying a +N chip. The implementation is straightforward. However, a key concern is the lack of accompanying tests. New features should always be tested to ensure correctness and prevent future regressions. Additionally, the number of visible chips is hardcoded, which could be improved by making it a configurable property for better flexibility.
nuke-ellington
left a comment
nuke-ellington
left a comment
There was a problem hiding this comment.
Visual tests are failing
Co-authored-by: Lukas Maurer <lukas.maurer@siemens.com>
…mens/ix into feature/3720_select-overflow
…mens/ix into feature/3720_select-overflow
lzeiml
force-pushed
the
feature/3720_select-overflow
branch
from
11ce6b3 to
eb0979e
Compare
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
Testcase:
<ix-select mode="multiple" style="width: 200px">
<ix-select-item value="1" label="1"></ix-select-item>
<ix-select-item value="2" label="2"></ix-select-item>
<ix-select-item value="3" label="3"></ix-select-item>
<ix-select-item value="4" label="4"></ix-select-item>
<ix-select-item value="5" label="5"></ix-select-item>
<ix-select-item value="6" label="6"></ix-select-item>
</ix-select>
<script>
const select = document.querySelector('ix-select');
select.value = ['1', '2', '3', '4'];
</script>
Result:
- No interaction:
- Dropdown opened once
There was a problem hiding this comment.
The +1 implementation is part of the select but if I have #2439 in mind it would make more sense to implement this as a share codebase e.g as mixin
| ref={(el) => { | ||
| if (el) { | ||
| this.chipElementRefs.set(item.value, el as HTMLElement); | ||
| } else { | ||
| this.chipElementRefs.delete(item.value); | ||
| } |
There was a problem hiding this comment.
I dont be happy with the calculation of state related variables inside the render cycle itself. The only reason this will no lead into a infinite re-rendering is the raf-function.
Please find a better solution to calculate the filter chips maybe not during the render cycle itself
| <div class="chips"> | ||
| <div | ||
| class="chips" | ||
| ref={(el) => (this.chipsEl = el as HTMLDivElement)} |
There was a problem hiding this comment.
We use normal a MakeRef for this any thoughts on your side you didnt choose it?
| variant="subtle-tertiary" | ||
| oval | ||
| size="16" | ||
| ref={(ref) => (this.clearButtonEl = ref ?? undefined)} |
There was a problem hiding this comment.
We use normal a MakeRef for this any thoughts on your side you didnt choose it?
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
Accessibility support is missing at all.
- "+1" -chip can not be accessed via keyboard
- Chips and "+1" not visible in the accessibility tree
- Accessibility testing component-test / framework-tests (aria snapshots) missing
There was a problem hiding this comment.
Pull request overview
Adds an overflow “+N” chip to ix-select in mode="multiple" so that, when selected chips exceed the available width, only the visible chips are rendered and the remainder is summarized.
Changes:
- Implement chip overflow measurement/rendering in
packages/core/src/components/select/select.tsxand adjust layout inselect.scss. - Add Playwright component tests for the overflow chip behavior and update preview examples to select 3 items at a constrained width.
- Update visual-testing fixtures and refresh screenshot baselines.
Reviewed changes
Copilot reviewed 10 out of 77 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/src/components/select/select.tsx |
Adds overflow chip logic, chip measurement, and Enter-key handling for editable mode. |
packages/core/src/components/select/select.scss |
Switches chips layout to single-line overflow behavior (no wrap/scroll). |
packages/core/src/components/select/test/select.ct.ts |
Adds CT coverage for overflow chip in multiple + editable multiple modes; updates some mounts. |
testing/visual-testing/tests/select/mode-multiple-overflow/index.html |
Adjusts fixture width and adds a long label case for overflow visuals. |
packages/*-test-app*/src/preview-examples/select-multiple.* |
Constrains width and updates initial selection to 3 items to demonstrate “+N”. |
testing/visual-testing/__screenshots__/** |
Updates visual regression baselines for select (and one pagination baseline). |
.changeset/stupid-sheep-grin.md |
Declares a minor release note for the feature. |
Comments suppressed due to low confidence (1)
packages/core/src/components/select/select.tsx:1166
- The chevron button ref callback assigns
this.chevronButtonEl = ref!. Stencil ref callbacks may receivenullduring teardown/re-render; using a non-null assertion can leavechevronButtonElasnullat runtime (despite being typed asundefined) and makes the intent unclear. Preferthis.chevronButtonEl = ref ?? undefinedfor consistency withclearButtonEland safer typing.
ref={(ref) => {
this.chevronButtonEl = ref!;
const element = ref as unknown as HTMLButtonElement;
// VDOM issue if tabIndex is provided via property <ix-icon-button tabIndex={-1}>
// the tabindex will be '0' after expanding the dropdown
element.tabIndex = -1;
element.ariaHidden = 'true';
}}
| this.handleUpdateSelectionMutlipleMode(); | ||
| } | ||
|
|
||
| private handleUpdateSelectionMutlipleMode() { | ||
| if (!this.isMultipleMode) { | ||
| return; | ||
| } | ||
|
|
||
| const currentValues = new Set(this.selectedItems.map((i) => i.value)); | ||
| for (const key of this.chipWidths.keys()) { | ||
| if (!currentValues.has(key)) this.chipWidths.delete(key); | ||
| } | ||
| if (this.pendingChipValue === null) { | ||
| this.calculateOverflow(); | ||
| } |
There was a problem hiding this comment.
handleUpdateSelectionMutlipleMode appears to have a typo in its name ("Mutliple"). Please rename to handleUpdateSelectionMultipleMode (and update the call site) to avoid propagating the misspelling into the public codebase and improve maintainability/searchability.
| event.stopPropagation(); | ||
|
|
||
| if (this.isMultipleMode) { | ||
| this.emitAddItem(this.inputFilterText); |
There was a problem hiding this comment.
onInputKeyDown always calls emitAddItem() in multiple mode on Enter, even when the typed text matches an existing item. This can create duplicate ix-select-item entries (same label/value) instead of selecting/toggling the existing item. Consider mirroring the single-mode behavior: if an existing item matches, call itemClick(existingItem.value); otherwise add a new item (or only add when isAddItemVisible() is true).
| this.emitAddItem(this.inputFilterText); | |
| const existingItem = this.itemExists(this.inputFilterText); | |
| if (existingItem) { | |
| this.itemClick(existingItem.value); | |
| } else { | |
| this.emitAddItem(this.inputFilterText); | |
| } |
| if (!this.editable || !this.inputFilterText) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
When handling Enter in onInputKeyDown, the event is stopped from propagating but not prevented. In a form context, pressing Enter in the input can still trigger a form submit/default action. Consider calling event.preventDefault() alongside stopPropagation() when you handle the key to avoid unintended submits/side-effects.
| event.preventDefault(); |
| const chevronWidth = this.chevronButtonEl?.offsetWidth ?? 0; | ||
| const clearWidth = this.clearButtonEl?.offsetWidth ?? 0; | ||
| const chipGap = 4; | ||
| const reservedInputSpace = 40; | ||
|
|
||
| const computeVisibleItems = (extraReservedWidth: number) => { | ||
| const availableWidth = | ||
| this.chipsEl!.clientWidth - | ||
| chevronWidth - | ||
| clearWidth - | ||
| reservedInputSpace - | ||
| extraReservedWidth; | ||
| const visibleItems = new Set<string>(); | ||
| let usedWidth = 0; | ||
|
|
||
| for (const item of this.selectedItems) { | ||
| const width = (this.chipWidths.get(item.value) ?? 60) + chipGap; | ||
|
|
There was a problem hiding this comment.
calculateOverflow() relies on hard-coded layout constants (chipGap = 4, reservedInputSpace = 40, default chip widths 60 / 54). These values are tightly coupled to CSS (e.g., .trigger has min-width: 4rem and .chips > div has margins), so overflow calculation may drift with theme/font-size/CSS changes. Consider deriving spacing from computed styles (margins/min-width) or centralizing these values (e.g., CSS variables) so layout and measurement stay consistent.
| "@siemens/ix": minor | ||
| --- | ||
|
|
||
| In multiple-mode **ix-select** now displays "+<number>" for the number of items that are selected if more than two items have been selected. |
There was a problem hiding this comment.
The changeset text says the "+" indicator is shown "if more than two items have been selected", but the implementation shows the overflow chip based on available width (chips exceeding container width), which may not always correspond to "more than two". Please adjust the changeset wording to match the actual behavior (overflow-based summarization) to avoid misleading release notes.
| In multiple-mode **ix-select** now displays "+<number>" for the number of items that are selected if more than two items have been selected. | |
| In multiple-mode **ix-select** now displays "+<number>" to summarize selected items that overflow the available width. |
|
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Closing in favor of new PR with more solid fundamentals: |
|
💡 What is the current behavior?
Currently overflow of selected item if ix-select is set to mode="multiple" is just handled by a scrollbar that appears.
🆕 What is the new behavior?
After two items have been selected, the third and every subsequent item will be indicated by a number after the first two selected items:
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)