Repository navigation
fix(tags): carry tag state in an icon, not the fill - #8569
Closed
talissoncosta wants to merge 19 commits into
Closed
talissoncosta wants to merge 19 commits into
talissoncosta wants to merge 19 commits into
Conversation
talissoncosta
requested review from
kyle-ssg
and removed request for
a team
September 22, 2026 13:17
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
the
front-end
Contributor
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 |
talissoncosta
force-pushed
the
fix/tags-accessibility-8465
branch
from
September 22, 2026 13:23
4d9d02b to
85e9d84
Compare
Contributor
❌ 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 |
Contributor
Visual Regression18 screenshots compared. See report for details. |
talissoncosta
force-pushed
the
fix/tags-accessibility-8465
branch
from
September 22, 2026 13:26
85e9d84 to
2d23e41
Compare
talissoncosta
force-pushed
the
fix/tags-accessibility-8465
branch
from
September 22, 2026 13:32
5ab4fa7 to
d59d045
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>
A tag stores a single colour and cannot know which page it will be drawn on. As a fill that is unsolvable: a fill carries the label's contrast as well as its own identity, so the Content tints are 11.3:1 to 15.8:1 against the dark page and glare there, while as a border on the light page they are 1.14:1 to 1.59:1 and vanish. The same eleven values work if the treatment follows the ground. In light they are the fill under a fixed dark ink, which is the design system's frame as handed off: 9.64:1 at worst. In dark there is no fill, the label takes the page's own ink, and the tint becomes the border: 11.32:1 at worst, where a border needs 3:1. One stored value per tag, the design system's palette throughout, nothing derived. What makes these colours work is what made them look unusable: they are pale, which is right behind dark text and right as a border on a dark page. The chip reads its border colour from a custom property, because Chip.scss is injected by the component at runtime and would otherwise beat any border-color a utility class sets on the same element. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
talissoncosta
force-pushed
the
fix/tags-accessibility-8465
branch
from
September 29, 2026 11:17
a6bace8 to
f232f9f
Compare
Two things kept the new treatment off the screen. `border-0` was on every non-system tag, from when the fill carried the colour on both pages and an outline would have doubled the edge. It is Bootstrap's, so `border: 0 !important`, which beats the chip's own rule whatever the source order. In dark the fill is transparent and the border is the whole treatment, so the tag rendered as bare text. The light theme sets the border token to transparent, so nothing doubles. The tag in the picker sat directly inside a column flex container and stretched to its full width, reading as a text field rather than a chip. `Tag` also worked out `disabled` itself, from the plan the account is on. That is the caller's to know, and reaching for it pulled the whole utils module into anything that renders a tag, which is why the component could not be storied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tag.color is an unvalidated CharField, so a tag created through the API or an import can hold any string, and a lookup table of the colours the picker has offered left those falling through to a neutral fill. Matching on hue answers for every colour and needs no migration. It also keeps the eleven swatches the only colours a tag can take, so contrast stays settled by the palette rather than by whatever was stored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ToggleChip was Chip with a selection mark, so it becomes a prop on Chip rather than a second component. `selected` rings the chip and fades it when it is not chosen, which suits a cloud of chips where the chip is the whole control; lists mark their rows instead. The default height comes from the design system's chip frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tag resolved a plan entitlement through AccountStore on every render and read a feature flag to decide whether to render at all. A chip that reports on an organisation's billing plan cannot be storied and cannot be tested without a Flux store. `disabled` is a prop now, and the feature-health gate moves to TagValues, the display path five other components render through. Both were also computing `disabled` separately and each applying opacity-50, so a disabled stale tag faded twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Selection was drawn inside the tag: the chip carried a checkbox, so its fill ran behind a control that is not part of it. A tag is a colour and a name; whether it is picked is a fact about the row. TagRow marks it with a trailing checkmark, as assignees, groups, roles and the table filters all do. Anything in `trailing` grows in at the right edge on hover and on focus, with the mark sliding left: this is the first list carrying both a mark and a row menu, and with both always present the menu ends up stranded mid-row. AddEditTags is the first caller, and its permanent red trash icon becomes a menu. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row came from the redesign branch, where DropdownMenu takes a trigger and the colour picker reads the colour back off the tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dragos's dark-mode frame keeps the eleven tints as the fill with the same dark label, identical to light. Only the primary chip inverts, and that is not a tag. So the theme branch goes: one surface, one ink, one border per tag, and no dark override. The earlier reading, that a pale fill glares on a dark page and has to become a stroke, cost the tags their identity, since eleven pale 1px lines on navy are not tellable apart. The ink is the palette's own always-dark rather than slate-600, which was a guess. The label sits on the tag's fill, never on the page, so its contrast is fixed by the palette and cannot move with the theme. The fill reads 1.18:1 against white, under the 3:1 for non-text. That is the design as handed off, and it holds up: a tag is read from its label, which clears AA on every tint, not from its edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The menu is drawn through a portal on the body, so it is not a descendant of whatever opened it. Anything watching for a click outside itself, an InlineModal holding the menu, say, counted a click on a menu item as one and closed: picking Edit on a tag opened the edit form and shut it again a moment later, because that watcher defers its close by 100ms. The menu belongs to its trigger wherever it happens to be drawn, so the event now stops at the menu's root. The listener is a native one, not React's onMouseUp, since React's sit below document and the watchers are on document itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chip spaces its own children, but the label and the icon share one span, so the lock on a permanent tag sat against the last letter. The ionicon it replaced carried its own margin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 29, 2026
Contributor
Author
|
Superseded by a three-PR stack, so each piece can be reviewed for what it is:
Closing this one. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
already stored by OKLCH hue, so no tag changes identity and nothing migrates. The old lookup
was an exact-hex table, so anything stored outside its twenty values got no treatment at all.
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.
The tag list also gets a row, so selection is a mark on the row rather than a checkbox drawn
inside the chip, and the per-tag actions move into a menu.
Three things worth knowing:
no dark override. An earlier version of this branch turned the fill into a stroke for dark, on
the reasoning that a pale fill glares on a dark page. That cost the tags their identity, since
eleven pale 1px lines on navy are not tellable apart, and it was solving a problem the frame
does not have.
handed off and it holds up: a tag is read from its label, which clears AA on every tint.
to tags, most visible thing here.
The lock on a permanent tag comes back
It will look like a new icon. It isn't: it has been dead on main since #4822 (Nov 2024), which
rewrote the call to wrap the colour argument and dropped the last one.
renderIconstill declared four parameters, soisPermanentwasundefinedand the lock branchalways returned null. @kyle-ssg had wired it up correctly 13 days earlier in #4804. The tooltip
still explained deletion protection throughout, so only the marker was missing.
Not here: migrating the ~60 remaining legacy
.chipsites, which needs its own issue. Five typeerrors came across with the tag row and are unfixed, all pre-existing in shape (
projectin thecreate payload,
Partial<Tag>whereTagis wanted, oneprojectIdstring/number).How did you test this code?
Storybook in both themes, plus
tagSwatches.test.tsasserting every label clears 4.5:1 on itsown fill and that no two tints collide.
tscunchanged against main.To check manually:
here and is fixed, the menu portals to the body so the panel counted a click on a menu item as
a click outside itself.
StatusBadgeshape change.