Skip to content

feat: Add chipCount prop to limit the amount of chips shown - #4805

Merged
pomfrida merged 1 commit into
mainfrom
feat/RHENRI/add-chipcount
Apr 14, 2026
Merged

feat: Add chipCount prop to limit the amount of chips shown#4805
pomfrida merged 1 commit into
mainfrom
feat/RHENRI/add-chipcount

Conversation

@yusijs

@yusijs yusijs commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

As discussed on slack

This PR adds a new prop to the Autocomplete-component (chipCount), which allows limiting the amount of chips dynamically, while retaining the existing behaviour if left unspecified.

@yusijs
yusijs force-pushed the feat/RHENRI/add-chipcount branch 2 times, most recently from 302563f to d999768 Compare April 13, 2026 12:20
@pomfrida

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Apr 13, 2026

Copy link
Copy Markdown

Claude finished @pomfrida's task in 1m 39s —— View job


Code Review

  • Gather context
  • Read changed files
  • Analyze changes and provide review feedback

The feature is well-scoped and the core slicing logic (slice(0, chipCount)) is correct — when chipCount is undefined, slice(0, undefined) returns the full array so existing behaviour is preserved. Tests cover the happy path. A few issues worth fixing before merging:


Bug: Focus management breaks with keyboard navigation

MultipleInput.tsx, handleChipRemove

Before this PR, index in the map callback matched a position in the full selectedItems array. After this PR it's the position within the slice (0 to chipCount - 1). The isLastChip check still compares against selectedItems.length - 1, which will almost always be false for visible chips when chipCount is set. More importantly, when the user presses Enter/Delete on the last visible chip, the code tries to focus selectedItems[index + 1] — but that item is hidden behind the "+N more" chip, so chipRefs.current.get(...) returns undefined and focus silently disappears. This is a WCAG 2.1 AA failure (Focus 2.4.3).

Suggested fix: after removing a visible chip, fall back to inputRef.current?.focus() when the intended next chip has no registered ref.

const handleChipRemove = (item: unknown, index: number, isKeyboardEvent: boolean) => {
  if (isKeyboardEvent && selectedItems.length > 1) {
    const isLastChip = index === selectedItems.length - 1
    const nextItem = selectedItems[isLastChip ? index - 1 : index + 1]
    const nextRef = chipRefs.current.get(getLabel(nextItem))
    // Fall back to input when next chip is hidden (beyond chipCount)
    if (nextRef) nextRef.focus()
    else inputRef.current?.focus()
  } else if (!isKeyboardEvent) {
    inputRef.current?.focus()
  }
  removeSelectedItem(item)
}

Fix this →


Wrong @default in JSDoc

Autocomplete.tsx, line 95–98

/** Amount of values to list in chips (only relevant if selectionDisplay = 'chips')
 * @default 'summary'   // ← copy-paste from the prop above, makes no sense
 */
chipCount?: number

'summary' is the default of selectionDisplay, not chipCount. The @default tag should be removed (since the real default is undefined / no limit).

Fix this →


Duplicated inline styles on the "+N more" chip

MultipleInput.tsx, lines 122–134

The style object on the "+N more" <Chip> is a copy of the one already applied to each regular chip. If the chip styling ever changes, it now has to be updated in two places. Consider extracting a shared constant:

const chipStyle = {
  outline: '1px solid var(--eds_interactive_primary__resting, rgba(0, 112, 121, 1))',
  ...(density === 'compact' && { height: '16px', fontSize: '12px', gridGap: '0px' }),
}

and using style={chipStyle} in both chip renders.


Edge case: chipCount={0}

slice(0, 0) produces an empty array, so no chips are rendered. The "+N more" guard is chipCount && selectedItems.length > chipCount — since 0 is falsy, the "+N more" chip is also suppressed. The result is all selections are silently hidden with no visible indicator. Either clamp chipCount to >= 1, or treat chipCount={0} as "no limit" (same as undefined).


Minor: doc string missing backtick formatting

Autocomplete.docs.mdx

"use the chipCount prop"

Should be "use the chipCount prop" for consistency with the surrounding formatting.


Test placement

The new test is placed inside describe('Autocomplete: Scroll position and navigation memory', ...) which is semantically wrong — chip count has nothing to do with scroll/navigation memory. It would fit better in a describe('Autocomplete: Multiple selection and chips', ...) block.


Overall: The happy-path logic and test are solid. The focus-management regression and the wrong @default are the two things I'd want addressed before this merges.

@pomfrida

Copy link
Copy Markdown
Collaborator

Nice work @yusijs please check Claude review - I'll review it tomorrow myself as well

@yusijs
yusijs force-pushed the feat/RHENRI/add-chipcount branch from d999768 to f5edfa4 Compare April 13, 2026 19:31
@yusijs

yusijs commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the comments from Claude @pomfrida. I didn't split the style on the chips into a variable, because I don't think they would necessarily need to be kept in sync (and it's only 2 lines).

@yusijs
yusijs force-pushed the feat/RHENRI/add-chipcount branch from f5edfa4 to bcafe66 Compare April 14, 2026 05:45

@pomfrida pomfrida left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good! The core logic is solid and the focus fallback handles the edge cases. A couple of minor notes:

  1. isLastChip checks against the full selectedItems array, so when chipCount is set it'll never be true for the last visible chip — meaning keyboard-deleting the last visible chip always falls back to focusing the input instead of the previous chip. Not a bug (focus doesn't get lost), but slightly different UX compared to the no-chipCount case. Something to be aware of if you want to polish it later.

  2. The test is inside the "Scroll position and navigation memory" describe block — would read better in a chip-related block, but no big deal.

@pomfrida
pomfrida merged commit e0c44ed into main Apr 14, 2026
12 checks passed
@pomfrida
pomfrida deleted the feat/RHENRI/add-chipcount branch April 14, 2026 11:52
@github-actions github-actions Bot mentioned this pull request Apr 14, 2026
@github-actions github-actions Bot mentioned this pull request May 20, 2026
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