feat(chip): semantic variants, pill shape and a status dot - #8505
talissoncosta wants to merge 4 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe shared Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The PR adds shared status variants and migrates StatusBadge to Chip. Current risk is low: the component documentation is slightly misleading and a stylesheet validation concern remains unresolved, but no runtime or data-impacting failure is indicated. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
front-end
Docker builds report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 83966be4-9c40-4d53-a8de-9374526e10d9
📒 Files selected for processing (7)
frontend/documentation/components/Chip.stories.tsxfrontend/web/components/base/Chip/Chip.scssfrontend/web/components/base/Chip/Chip.tsxfrontend/web/components/base/Chip/ChipDot.tsxfrontend/web/components/base/Chip/index.tsfrontend/web/components/experiments/StatusBadge/StatusBadge.scssfrontend/web/components/experiments/StatusBadge/StatusBadge.tsx
💤 Files with no reviewable changes (1)
- frontend/web/components/experiments/StatusBadge/StatusBadge.scss
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| width: 6px; | ||
| height: 6px; | ||
| border-radius: var(--radius-full); | ||
| background: currentColor; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the configured currentcolor keyword spelling.
Stylelint reports currentColor as a value-keyword-case error. This prevents the touched stylesheet from passing the configured lint check.
Proposed fix
- background: currentColor;
+ background: currentcolor;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background: currentColor; | |
| background: currentcolor; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 31-31: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
Source: Linters/SAST tools
| const ChipDot: FC = () => <span className='ds-chip__dot' aria-hidden='true' /> | ||
|
|
||
| export default ChipDot |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move ChipDot into its required component folder.
The new ChipDot component does not follow the required component layout. Create ChipDot/ChipDot.tsx, co-locate ChipDot.scss, and add ChipDot/index.ts.
frontend/web/components/base/Chip/ChipDot.tsx#L4-L6: move the component intoChipDot/ChipDot.tsxand move its styles intoChipDot/ChipDot.scss.frontend/web/components/base/Chip/index.ts#L2-L2: re-exportChipDotthrough the newChipDot/index.tsbarrel.
📍 Affects 2 files
frontend/web/components/base/Chip/ChipDot.tsx#L4-L6(this comment)frontend/web/components/base/Chip/index.ts#L2-L2
Source: Coding guidelines
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20352 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #20352 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20352 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20352 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20351 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20351 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20351 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20351 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #20349 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #20349 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #20349 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #20349 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
Completes the variant set so batch 4 of the consolidation does not need a second variants PR. - `danger` maps to the existing surface/text danger tokens. - `solid` is the one filled variant, on --color-surface-action. Uses `text-white` rather than a token because there is no inverse-text token yet; white on that surface is 5.93:1, so AA but not AAA. Both get a transparent border like the other status variants, so a filled chip keeps the same height as a bordered one beside it. Deliberately not covered: the app has a second, darker solid (`bg-primary900`, used by BetaFlag and PlanBasedAccess) alongside the `bg-primary` one this matches. Which of the two survives is a design decision, not a mapping. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
feature
The Figma tags frame is the source of truth and specifies a 6px radius on a fixed 24px height. We had `rounded-sm` (4px) and no explicit heights at all. - Non-pill chips move to `rounded-md`. Affects all 14 existing ds-chip call sites, which is the intent. - `--xs` states `height: 24px` rather than deriving it from the inherited line-height. The frame's 8px vertical padding is not applied: it is an artefact of a height override on the auto-layout and would leave 8px for a 12px label. `--xs` is the size the design maps to; the tag surfaces the frame covers (feature rows, TagValues) all pass the xs modifier, and the legacy `.chip--xs` was already 24px. `pill` keeps `rounded-full`, preserving the shape StatusBadge.scss already had on main. That is now the only divergence from the frame's 6px, on a surface the frame does not cover, so it is worth confirming with design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
feature
Nothing in the tags frame is fully rounded, and the frame is the source of truth. `pill` existed only to preserve the `--radius-full` that StatusBadge.scss carried on main, which had never been checked against the design system. So every chip is now 6px. `StatusBadge` and the status/count stories drop the prop, and `Chip` has no shape axis to get wrong. `ds-chip__dot` keeps `--radius-full`: it is a 6px circle, not a chip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
feature
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: f75c98c4-24dd-4f22-be74-127fab2d351d
📒 Files selected for processing (3)
frontend/documentation/components/Chip.stories.tsxfrontend/web/components/base/Chip/Chip.tsxfrontend/web/components/experiments/StatusBadge/StatusBadge.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
| description: { | ||
| component: | ||
| 'Canonical token-based chip primitive: a small labelled pill token. Layout via Bootstrap utilities, colour/radius via token utilities, padding/sizes/border/truncation in SCSS. Leading/trailing icons go in as children. Selection lives in ToggleChip and count badges are a separate Badge concern. The legacy `.chip` (old SCSS vars + manual dark-mode block, ~35×) migrates onto this under #6606.', | ||
| 'Canonical token-based chip primitive: a small labelled pill token. Layout via Bootstrap utilities, colour/radius via token utilities, padding/sizes/border/truncation in SCSS. Leading/trailing icons go in as children. `variant` covers neutral, accent, the five status colours and `solid`; `ChipDot` adds the leading dot in `currentColor`. Radius is a fixed 6px from the tags frame, so there is no shape prop. Selection lives in ToggleChip. The legacy `.chip` (old SCSS vars + manual dark-mode block, ~35×) migrates onto this under #6606.', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace the inaccurate “pill token” description.
The fixed 6px radius does not produce a pill shape. Change “pill token” to “chip” or “labelled token” so the description matches the component API and rendered shape.
1 participant
docs/if required so people know about the feature.Changes
No issue yet — the chip consolidation still needs one raising. Related: #8465 covers the tag half.
Groundwork so the 58 legacy
.chipmarkup sites have somewhere to go. On its own this changesalmost nothing users see;
StatusBadgeis the only consumer.ChipVariantgainssuccess,warning,infoandmuted, wired throughVARIANT_UTILITIESas
bg-surface-*+text-*token utilities, so they follow the theme with no.darkblock.pillprop swapsrounded-smforrounded-full, for status and count shapes.ChipDot(ds-chip__dot), the decorative leading dot on a status chip.aria-hidden,since the label carries the meaning.
StatusBadgerebuilt onChip: 51 lines of bespoke SCSS deleted, the component down to astatus-to-variant map.
Why it matters beyond
StatusBadge: the legacy.chipcannot be retired while it is the onlything that can express a solid, status or removable pill. Four shapes in that CSS
(static status, removable, solid brand, semantic status) need a variant before their call sites
can move, and this covers the semantic-status one.
ChipVariantnow coversneutral,accent,solidand the five status colours(
success,warning,danger,info,muted), so batch 4 does not need a second variants PR.dangermaps to the existing surface/text danger tokens;solidis the one filled variant, on--color-surface-action, usingtext-whitebecause there is no inverse-text token yet (5.93:1,so AA but not AAA).
The Figma tags frame is the source of truth for shape, and we were off it: it specifies a 6px
radius on a fixed 24px height, we had
rounded-sm(4px) and no explicit heights. Non-pill chipsnow use
rounded-md, which changes all 14 existingds-chipcall sites, and--xsstatesheight: 24px.--xsis the size the frame maps to: the surfaces it covers (feature rows,TagValues) all pass the xs modifier, and the legacy.chip--xswas already 24px. The frame's8px vertical padding is not applied — it is an artefact of a height override on the auto-layout
and would leave 8px for a 12px label.
pillis gone. Nothing in the frame is fully rounded, and the prop existed only to preserve the--radius-fullthatStatusBadge.scsscarried on main, which had never been checked against thedesign system. Every chip is now 6px and
Chiphas no shape axis to get wrong.ds-chip__dotkeeps
--radius-fullbecause it is a 6px circle, not a chip.This changes the experiment status badges from fully rounded to 6px. Deliberate, per the
frame, but it is the most visible thing in this PR.
Two things deliberately left out:
bg-primary900(BetaFlag,PlanBasedAccess) andbg-primary(UsageTab,DerivedPermissionsList,CompareFeatureRow,FeatureTags).solidmatches the latter. Which of the two survives is a design decision, nota mapping, so batch 4 needs that answered first.
it is where the accessibility problem in Tags optimisation #8465 actually lives.
Also unresolved: the
#6606references inChip.tsxandChip.scssshould point at theconsolidation issue, which does not exist yet.
How did you test this code?
Static checks only so far — this needs a visual pass before merge:
npx tsc --noEmit: no new errors (923 onmain, 923 here; the pre-existingCannot find module 'storybook'errors also hit untouched story files).npx eslintclean on the touched paths.Still to do, and the reason this is worth a careful look rather than a rubber stamp:
Chip.stories.tsxis opted intoChromatic, so the CI snapshot will cover the variants themselves.
StatusBadgeon the Experiments list across all fourExperimentStatusvalues (created,running,paused,completed) in both themes, checking the pill shape and dot match thebespoke SCSS this deletes. Nothing snapshots
StatusBadge, so that one is manual.