Skip to content

fix(combobox): correct fit selection count chip#14243

Open
Amretasre002762670 wants to merge 4 commits intodevfrom
Amretasre002762670/12287-combobox-fit-display
Open

fix(combobox): correct fit selection count chip#14243
Amretasre002762670 wants to merge 4 commits intodevfrom
Amretasre002762670/12287-combobox-fit-display

Conversation

@Amretasre002762670
Copy link
Copy Markdown
Contributor

@Amretasre002762670 Amretasre002762670 commented Apr 10, 2026

Related Issue: #12287

Summary

Refined combobox fit-mode count chips and deferred selection refresh to keep counts accurate.

@Amretasre002762670 Amretasre002762670 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Apr 10, 2026
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Apr 10, 2026
@Amretasre002762670 Amretasre002762670 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Apr 10, 2026
jcfranco
jcfranco reviewed Apr 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Looking good, @Amretasre002762670! 😎

Do existing tests provide coverage for these changes or do they need to be updated?

Comment thread packages/components/src/components/combobox/combobox.tsx Outdated
private renderChipCount(count: number, scale: Scale): JsxNode {
const label =
this.messages.disabledSelectedCount?.replace("{count}", `${count}`) ?? `+${count}`;
private renderChipCount(count: number, scale: Scale, includePlus: boolean = true): JsxNode {
Copy link
Copy Markdown
Member

@jcfranco jcfranco Apr 13, 2026

Choose a reason for hiding this comment

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

Nit: includePlus is optional, but still passes a default value in one of its call sites. I'd suggest making this a required argument or not passing the default when calling it.

Comment thread packages/components/src/components/combobox/combobox.tsx Outdated
@jcfranco
Copy link
Copy Markdown
Member

jcfranco commented Apr 13, 2026 •
edited
Loading

@eriklharper Can you review these changes too?

@jcfranco jcfranco requested a review from eriklharper April 13, 2026 15:46
@Amretasre002762670
Copy link
Copy Markdown
Contributor Author

Amretasre002762670 commented Apr 13, 2026

Do existing tests provide coverage for these changes or do they need to be updated?

The current test suite covers these changes, so no additional updates are needed.
eb33a56

@Amretasre002762670 Amretasre002762670 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Apr 13, 2026
@eriklharper
Copy link
Copy Markdown
Contributor

eriklharper commented Apr 14, 2026 •
edited
Loading

Looks like there should be some extra space between this +1 and the placeholder-icon
image

Also, wondering why its "+1" instead of "Sequoia" in this instance when there's plenty of room to render the full label?

@eriklharper
Copy link
Copy Markdown
Contributor

eriklharper commented Apr 14, 2026 •
edited
Loading

I'm also wondering why disabled + selected options don't appear as one of the values in value or selectedItems?

image

I would expect them to appear in the component's item state because they're selected, and by being disabled it means that the user can't remove the option from the selection.

This appears to be pre-existing behavior, but I think it is worth revisiting...

@DitwanP DitwanP mentioned this pull request Apr 14, 2026
Open
11 tasks
@DitwanP
Copy link
Copy Markdown
Contributor

DitwanP commented Apr 14, 2026 •
edited
Loading

I'm also wondering why disabled + selected options don't appear as one of the values in value or selectedItems?

This appears to be pre-existing behavior, but I think it is worth revisiting...

Thanks Erik! I added this as an acceptance criteria for the related issue #12287 as it seems like it should be done in conjunction.

eriklharper
eriklharper requested changes Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@eriklharper eriklharper left a comment

Choose a reason for hiding this comment

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

I think if we fix the current behavior where the disabled + selected options get added to the combobox's value, a lot of the built-in chip display logic should "just work", so let's address that as part of this PR since it is directly related. @Amretasre002762670 we can pair on this if you're interested, just let me know!

@Amretasre002762670 Amretasre002762670 added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jcfranco jcfranco jcfranco left review comments

@eriklharper eriklharper eriklharper requested changes

@ashetland ashetland Awaiting requested review from ashetland

Requested changes must be addressed to merge this pull request.

Assignees

@Amretasre002762670 Amretasre002762670

Labels

bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Amretasre002762670 @jcfranco @eriklharper @DitwanP