feat(pill): align with Fusion DS#8038
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR updates the Pill component and its MultiSelect integration to align styling and behavior with the Fusion Design System, including new token-driven color variants and refreshed Storybook documentation/stories.
Changes:
- Introduces token-based
variant/inversestyling forPill, with deprecations for legacy props (colorVariant,isDarkBackground,pillRole,XLsize). - Updates
MultiSelectto render pills using the newvariantapproach and refreshes its stories/tests accordingly. - Revamps Pill Storybook stories/docs with Controls-driven Playground and updated examples.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/select/option/option.component.tsx | Adds variant and deprecates borderColor on OptionProps for MultiSelect pills. |
| src/components/select/multi-select/multi-select.component.tsx | Switches pill prop mapping from borderColor to variant when rendering selected values. |
| src/components/select/multi-select/multi-select.test.tsx | Updates assertions to match new token-based Pill styling. |
| src/components/select/multi-select/multi-select.stories.tsx | Updates MultiSelect stories to use variant-based pills. |
| src/components/select/multi-select/multi-select-test.stories.tsx | Updates MultiSelect test stories to use variant-based pills. |
| src/components/select/multi-select/multi-select-interaction.stories.tsx | Updates interaction story to use variant-based pills. |
| src/components/select/multi-select/multi-select.pw.tsx | Updates Playwright expectations for new pill colors/styles. |
| src/components/select/multi-select/components.test-pw.tsx | Updates PW component fixtures to set variants for pill rendering. |
| src/components/pill/pill.component.tsx | Adds variant, inverse, icon, and introduces deprecation logging/mapping logic. |
| src/components/pill/pill.style.ts | Reworks Pill styling to use new global/tier tokens and new delete-button styling. |
| src/components/pill/pill.style.config.ts | Replaces legacy semantic config with Fusion-aligned variant token mappings (incl. alt + inverse handling). |
| src/components/pill/pill.test.tsx | Updates unit tests for new styling and adds coverage for new deprecation warnings. |
| src/components/pill/pill.stories.tsx | Adds Playground story with Storybook Controls and updates examples for new API. |
| src/components/pill/pill.mdx | Updates docs to include Playground + Controls and revised examples/sections. |
| src/components/pill/pill.pw.tsx | Updates accessibility mount scenario to use inverse/variant. |
| src/components/carbon-provider/carbon-provider.pw.tsx | Removes Pill theme assertion from provider PW suite (no longer validating theme colors there). |
Files excluded by content exclusion policy (4)
- package-lock.json
- skills/carbon-react/components/multi-select.md
- skills/carbon-react/components/option.md
- skills/carbon-react/components/pill.md
f2e821a to
6b33d0f
Compare
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Files excluded by content exclusion policy (4)
- package-lock.json
- skills/carbon-react/components/multi-select.md
- skills/carbon-react/components/option.md
- skills/carbon-react/components/pill.md
23a1c5d to
5c26963
Compare
There was a problem hiding this comment.
suggestion: we should probably discard this change
| */ | ||
| borderColor?: string; | ||
| /** MultiSelect only - set Pill color variant */ | ||
| variant?: PillVariant; |
There was a problem hiding this comment.
suggestion: This might be slightly cleaner: - avoids the type declaration above as the type is not re-used anywhere else
variant?: PillProps["variant"];
There was a problem hiding this comment.
Agree with Tom it seems odd to make PillVariant NonNullable then make variant optional in OptionProps
| let isSelected = selectListContext.currentOptionsListIndex === index; | ||
| const internalIdRef = useRef(id || guid()); | ||
| const { | ||
| variant: _variant, |
There was a problem hiding this comment.
question: do we not need variantto pass down at any point here? As its destructured here but never actually used elsewhere, I understand this is probably intentional but just thought I'd ask, so I had context 👍
There was a problem hiding this comment.
This is intentional as we destructure variant (along with borderColor and fill) so those pill only props are not forwarded to the underlying option element via the rest. They are accepted for MultiSelect compatibility but not used by Option rendering itself
| ); | ||
| }, | ||
| parameters: { | ||
| chromatic: { disableSnapshot: false }, |
There was a problem hiding this comment.
suggestion (non-blocking): just noticed that we're setting { disableSnapshot: false } on every story apart from the last story (WithCustomAriaLabels).
Could we set { disableSnapshot: true } at the file root instead?
| : undefined) ?? | ||
| "grey"; | ||
|
|
||
| if (colorVariant !== undefined && !deprecateColorVariantTriggered) { |
There was a problem hiding this comment.
suggestion (non-blocking): We could just reply on the @depcreated annotation you've already used to communicate these deprecation warnings to consumers.
I definitely understand the fact that the annotation will not properly warn users who already use the props unless they closely inspect their JSX.
However, I've personally decided against using a logger in my own alignment work purely because of the added code and test overhead, but its completely up to you!
There was a problem hiding this comment.
Yeah my preference has been to avoid logging, our test output is getting kind of crazy atm, consumers are either ignoring or have pipeline checks agains logs so it's actually adding no value or even a cost etc
| }: PillProps) => { | ||
| const locale = useLocale(); | ||
| const resolvedPillRole = pillRole ?? "tag"; | ||
| const resolvedVariant = |
There was a problem hiding this comment.
suggestion (non-blocking): reckon something like this may be more readable?
function resolveVariant(variant, colorVariant) {
if (variant !== undefined) return variant;
if (colorVariant !== undefined) {
const mapped = LEGACY_COLOR_VARIANT_MAP[colorVariant];
if (mapped !== undefined) return mapped;
}
return "grey";
}
The nullish coalescence + ternary + nullish coalescence can be a bit difficult to read imo
There was a problem hiding this comment.
This would also work as using undefined to index to an object will just return undefined etc
const resolveVariant = variant ?? LEGACY_COLOR_VARIANT_MAP[colorVariant] ?? "grey";
| colorVariant={colorVariant} | ||
| isDarkBackground={isDarkBackground} | ||
| colorVariant={resolvedVariant} | ||
| inverse={inverse || isDarkBackground} |
There was a problem hiding this comment.
praise: good shout mapping isDarkBackground to inverse
| ## Examples | ||
|
|
||
| ### Default | ||
| ## Playground |
There was a problem hiding this comment.
praise: I actually really like the idea of a playground, especially for a component like Pill which doesn't rely on custom compositions etc.
If the other reviewer is happy with this approach, I say we keep it 👍
| pillRole: "tag" | "status"; | ||
| } | ||
|
|
||
| export const StyledDeleteButton = styled.button``; |
There was a problem hiding this comment.
suggestion, could we not use import the Button directly in pill.component.tsx as we aren't applying any styling directly here.
I can see its being used for specific targeting below, but I think we can achieve the same effect with a unique data tag on the Button in the component file and then target below like this, [data-role="delete"-button] { etc
There was a problem hiding this comment.
I think it'll be more effort to do that than using a standalone button here. I think if we're just creating an empty styled-component we could just target & > button instead though. Alternatively, we could pass the props to StyledDeleteButton and add the styling there. All this is non-blocking for me though as what you have will work fine
There was a problem hiding this comment.
There were a lot of issues with using the Button component, so it was safer and made more sense to create our own. I spoke to Ed about this one at the time
| }: PillProps) => { | ||
| const locale = useLocale(); | ||
| const resolvedPillRole = pillRole ?? "tag"; | ||
| const resolvedVariant = |
There was a problem hiding this comment.
This would also work as using undefined to index to an object will just return undefined etc
const resolveVariant = variant ?? LEGACY_COLOR_VARIANT_MAP[colorVariant] ?? "grey";
|
|
||
| export default (isDarkBackground: boolean): StyledPillConfig => { | ||
| export default (inverse: boolean): StyledPillConfig => { | ||
| const content = inverse |
There was a problem hiding this comment.
suggestion (non-blocking): you could leverage interpolation here
const content = `var(--pill-generic${inverse ? "-inverse" : ""}-label-default)`;
const contentAlt = `var(--pill-generic${inverse ? "-inverse" : ""}-label-alt-default)`;
|
|
||
| color: ${contentColor}; | ||
|
|
||
| ${StyledIcon} { |
There was a problem hiding this comment.
suggestion: it might be cleaner to invert this and add the styling in icon.style when it's inside a Pill etc
| pillRole: "tag" | "status"; | ||
| } | ||
|
|
||
| export const StyledDeleteButton = styled.button``; |
There was a problem hiding this comment.
I think it'll be more effort to do that than using a standalone button here. I think if we're just creating an empty styled-component we could just target & > button instead though. Alternatively, we could pass the props to StyledDeleteButton and add the styling there. All this is non-blocking for me though as what you have will work fine
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
| color: ${inFill || borderColor ? contentColor : contentAltColor}; |
There was a problem hiding this comment.
suggestion (non-blocking): you've got this conditional/ternary repeated a few times so you could assign it to a variable above and then re-use etc
| */ | ||
| borderColor?: string; | ||
| /** MultiSelect only - set Pill color variant */ | ||
| variant?: PillVariant; |
There was a problem hiding this comment.
Agree with Tom it seems odd to make PillVariant NonNullable then make variant optional in OptionProps
| isDarkBackground={isDarkBackground} | ||
| colorVariant={resolvedVariant} | ||
| inverse={inverse || isDarkBackground} | ||
| isDeletable={!!onDelete} |
There was a problem hiding this comment.
we should ensure these props are non-transitive so they don't end up on the underlying DOM elements
| isDeletable={!!onDelete} | |
| $inFill={fill} | |
| $colorVariant={resolvedVariant} | |
| $inverse={inverse || isDarkBackground} | |
| $isDeletable={!!onDelete} | |
| $size={size} | |
| $borderColor={borderColor} |
da538a0 to
18da1e5
Compare
7d11628 to
46f41cf
Compare
Proposed behaviour
PlaygroundCurrent behaviour
Checklist
QA
Additional context
Testing instructions