fix(tags): carry tag state in an icon, not the fill - #8569
talissoncosta wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
the
front-end
Docker builds report
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true📝 WalkthroughWalkthroughThe change adds paired light and dark tag surface/text tokens with contrast validation and generated utility classes. It expands Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to System tags can render with the wrong visual treatment, and assistive-technology users cannot tell whether an interactive tag is selected. Correct both before merging. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
4d9d02b to
85e9d84
Compare
❌ private-cloud · depot-ubuntu-latest-16 — run #20583 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss 🗂️ Previous results❌ private-cloud · depot-ubuntu-latest-16 — run #20582 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ oss · depot-ubuntu-latest-16 — run #20583 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ oss · depot-ubuntu-latest-16 — run #20582 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ private-cloud · depot-ubuntu-latest-16 — run #20581 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ private-cloud · depot-ubuntu-latest-arm-16 — run #20581 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ oss · depot-ubuntu-latest-16 — run #20581 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ private-cloud · depot-ubuntu-latest-16 — run #20580 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ oss · depot-ubuntu-latest-16 — run #20580 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ private-cloud · depot-ubuntu-latest-16 — run #20579 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss ❌ oss · depot-ubuntu-latest-16 — run #20579 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Failed testsfirefox › tests/flag-tests.pw.ts › Flag Tests › Feature flags can have tags added and be archived @oss |
Visual Regression18 screenshots compared. See report for details. |
85e9d84 to
2d23e41
Compare
5ab4fa7 to
d59d045
Compare
0957283 to
0d85b05
Compare
0d85b05 to
cb2f9e1
Compare
cb2f9e1 to
4bce3e4
Compare
The legacy `.chip` is the only thing in the app that can express a status pill, a solid brand pill or a removable one, which is what keeps its 46 call sites from moving. This gives Chip the colours they need. Variants are semantic, so a chip says what it means rather than which colour it is, and each resolves to bg-surface-*/text-* token utilities. `solid` is the one filled variant, on --color-surface-action with text-white, because there is no inverse-text token yet (5.93:1, AA but not AAA). Note the app has a second, darker solid (bg-primary900, BetaFlag and PlanBasedAccess) that this deliberately does not cover: which of the two survives is a design decision. ChipDot is the leading dot on a status chip, in currentColor so it follows the variant with nothing to wire, and aria-hidden since the label carries meaning. Shape comes from the Figma tags frame, which we were off: it specifies a 6px radius on a fixed 24px height, against rounded-sm (4px) and no explicit height here. The frame's 8px vertical padding is not applied, being an artefact of a height override that would leave 8px for a 12px label. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It was a bespoke pill with its own stylesheet, duplicating shape, sizing and a hand-written dark-mode block that Chip already handles. Rebuilt as a status-to-variant map, deleting 51 lines of SCSS. This changes the experiment status badges from fully rounded to 6px. Deliberate: --radius-full came from StatusBadge.scss and had never been checked against the design system, and nothing in the Figma tags frame is fully rounded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Custom tags derive their fill, border and text from the tag's own hue at
render time, so the contrast ratio is whatever the hue happens to give and
most of the palette fails WCAG AA.
Adds a scale of validated {surface, text} pairs instead, built from the
existing primitive ramps so a ramp change carries through:
light surface <hue>-100 text <hue>-800
dark surface <hue>-900 text <hue>-100
Two exceptions, both because the ramp offers no step that works. Gold's light
surface is too pale for -800 (2.82), so its text takes -950. Slate's dark
surface at -900 is the page background, so it takes -800.
Splitting them into tag-surface and tag-text lets the generator emit
.bg-tag-<hue> and .text-tag-<hue>, so a tag needs no stylesheet of its own.
The test proves AA once, over the scale, rather than measuring contrast in the
browser. The story shows each pair as a real chip captioned with its measured
ratio, in both themes, so the claim is checkable rather than asserted.
Nothing renders against these yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The design asks for a redrawn Stale, the ionicon being too detailed at 16px, and a PR dequeued icon. The latter is a live gap rather than a nicety: GitHubTag.PR_DEQUEUED is already written by the API when GitHub reports a PR leaving a merge queue, so that tag reaches the UI and renderIcon falls through to a bare return. Both follow the frame's spec, 16px at 1.5px stroke, and are drawn at 0.88 of the box so they carry the same optical weight as the fill-based Octicons beside them. pr-draft moves onto --color-icon-secondary. It was the only status icon on currentColor, so it took the chip's label colour: near-black in light, white in dark, neither of which the design asks for. Its siblings hardcode GitHub's brand colours, which are the same in either theme, but the design gives pr-draft #747B86, a neutral, and a neutral cannot be hardcoded: it has to lift in dark mode or it sinks into the surface. Both new icons are registered in the catalogue, which is a hand-maintained list, so an icon is invisible in Storybook without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tags rendered the tag's own colour as text on an 8% tint of itself, so the
contrast ratio was whatever the hue happened to give and most of the palette
failed WCAG AA.
System tags (Issue, PR, Stale, Unhealthy) now sit on plain surface with a
neutral border and a coloured icon, so the state is carried by the icon and
the label keeps full contrast. Custom tags take a validated swatch pair from
the scale, keyed on the colour already stored, so no tag changes identity and
nothing needs migrating.
That removes every colour computation, in all four places it had been copied
to, two of which no stylesheet could reach:
Tag.tsx fade(.92) / fade(.76) / darken(.1), plus shouldLighten
and a #344562 special case, both of which existed only
because the fill was the hue
TagContent.tsx darken(.1) for the icon, and the same three again as an
inline style string
ToggleChip.tsx the same three, on Tag's onClick path
FeatureAction.tsx the same three, in a tooltip string
The two string sites stay strings, being rendered through
dangerouslySetInnerHTML, but they now carry the same classes as the real chip
rather than their own copy of the rules.
renderIcon also now answers to GITLAB, which had never been wired. Harmless
while the fill carried the state; a regression once it does not.
TagType gains GITHUB and GITLAB to match the API enum. The icon switch has
always branched on values the type said could not occur.
escapeHTML's character class is restated as the characters it keeps. Same set,
proven equivalent across U+0000..U+1FFF, but without control characters in the
literal, which the pre-commit lint rejects now the file is touched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers the semantic variants and the solid fill, then the two shapes a tag takes: a system tag on plain surface with a coloured icon, and a custom tag carrying a swatch pair. The tag stories live here rather than beside Tag because there is no tag component to story. Tag reads the store and the flags, and the appearance decision lives in tagSwatch.ts, which is pure and tested on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The picker offered twenty colours that rendered as eleven or so distinct tints. Of the 190 pairs, 26 were under dE 10 and twelve under dE 5; coral and maroon sat at 0.51, which is the same colour. Content is eleven colours, and the rate of indistinguishable pairs drops from 13.7% to 5.5%. So the swatch scale becomes his eleven, added as content-* primitives, and the twenty stored colours map onto them by nearest hue. Of the eleven pairs that now share a swatch, nine were already indistinguishable. Only two gave up a real difference, orange with amber and silver with slate, both at about dE 10.9. The colours are fixed rather than theme-aware. A tag chip carries its own surface, so it does not follow the page the way a panel does, which is why his palette has no dark values: it does not need any. That removes the per-swatch surface and text tokens entirely. One dark ink serves all eleven at 9.64:1 in the worst case, so the utilities pair a Content fill with slate-600. contentColours is exported from tokens.ts for anything that needs to iterate the palette, and TagSwatch is derived from it rather than repeating the names. tagColors keeps one representative per Content colour, the closest hue match of the old twenty, so the picker offers eleven choices with eleven outcomes rather than twenty choices with eleven. Tags keep the hex already stored on them, so nothing needs migrating and a colour set outside the picker still falls through to the neutral. Still open with design: Content has a 56 degree hue gap between Light-yellow and Light-peach, which is where amber fell, and no indigo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each swatch is a Tag rendered with no label. Nothing sized it, so it collapsed to the chip's minimum width and read as a narrow sliver rather than a colour. The wrapper it used, .tag--select, had no styles anywhere. CreateEditTag and ColourSelect rendered the same grid, so this becomes one TagColourPicker with its own stylesheet rather than a shared partial. The swatch is a 28px square, and the grid owns its gap so the me-1 that Tag adds for tags in a row is dropped here; Bootstrap emits spacing utilities with !important, which is why that needs one back. The sliver was always there. It showed up now because the Content colours are pale, where a saturated sliver still read as a colour chip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Neither had a story. The Chip stories cover how a tag looks by calling getTagSwatchUtilities themselves, so they would still pass if Tag broke, and the picker had nothing at all. Tag: a custom tag, every colour in the scale, the four system tags carrying state in an icon, a colour the picker never offered falling through to the neutral, the dot form, and selection. TagColourPicker: the grid with and without a selection, plus a live one. The picker story is the one that pays for itself. Its swatches were rendering as slivers because nothing sized them, twice over: the wrapper class had no styles anywhere, and the replacement targeted .chip when Chip renders .ds-chip. Both would have been visible here immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4bce3e4 to
a6bace8
Compare
This branch was successfully deployed
2 participants
docs/if required so people know about the feature.Changes
Contributes to #8465. Supersedes #8505, whose commits are included here.
Tags rendered the tag colour as text on an 8% tint of itself, so contrast was whatever the hue
gave and most of the palette failed AA.
icon, so the icon carries the state.
{surface, text}pair, keyed on the colour already stored, so notag changes identity and nothing migrates.
This deletes the colour maths from all four places it had been copied to, two of them inline
style strings behind
dangerouslySetInnerHTML.Chipgains the semantic variants those callsites need and
StatusBadgemoves onto it, dropping 51 lines of SCSS. Adds a redrawn Stale iconand PR dequeued, which the API already creates but the UI rendered without an icon.
Three things worth knowing:
is open with Dragos. Swapping them in is a
tokens.jsondiff, no component work.to tags, most visible thing here.
Not here: migrating the ~60 remaining legacy
.chipsites, which needs its own issue.How did you test this code?
Storybook in both themes, plus
tagSwatches.test.tsasserting every swatch pair clears 4.5:1 inlight and dark.
tsc910 errors against 913 on main. Not yet exercised on real screens.To check manually:
StatusBadgeshape change.