Skip to content

refactor(chip): fold ToggleChip into a selected prop - #8604

Closed
talissoncosta wants to merge 14 commits into
fix/tags-accessibility-8465from
refactor/chip-selected-prop
Closed

refactor(chip): fold ToggleChip into a selected prop#8604
talissoncosta wants to merge 14 commits into
fix/tags-accessibility-8465from
refactor/chip-selected-prop

Conversation

@talissoncosta

@talissoncosta talissoncosta commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Changes

Stacked on #8569.

Structural cleanup of the chip and tag components, plus the sizing from the chip frame in Figma.

ToggleChip becomes a selected prop on Chip. It was 40 lines wrapping Chip to add one thing, a tick when active, plus a conditional that existed only for the tag colour picker. children is now optional, because a selected chip with no children is exactly that bare swatch. A selected checkbox fills with the chip's ink and the tick takes the chip's own fill, a pairing already checked at 9.64:1 or better.

Chip takes its default size from the frame. 32px with an 8px gap, where ours was ~29px with a 4px gap. Tag was pinned to the smallest size, which is why tags read as undersized. --sm gains an explicit height so it does not inherit the new base and grow its seventeen call sites.

A checkbox appears only where a tag is selectable. Tag inferred this from !hideNames && !!onClick, so the tags already applied to a feature rendered an unchecked box against tags that are applied. The caller now says which kind of list it is. hideNames reads nothing after that and goes, along with the two components forwarding it.

VCSProviderTag moves out of tags/. It isn't a tag — it doesn't use Tag or Chip, it renders its own markup with a count and a provider icon.

Bugs this fixed on the way

All three came from the same inference in Tag, which encoded "is this a selection list?" in a prop about names:

  • a clickable tag without a name could never show selection
  • the branch sat above the feature_health guard, so a clickable unhealthy tag rendered with the flag off
  • applied tags showed an unchecked checkbox

What I deliberately left alone

StatusBadge, ControlWeightChip and SegmentMembershipBadge look redundant but aren't. Each names a domain concept and keeps its mapping in one place; collapsing them into Chip props would push experiment and segment knowledge into a primitive.

InlinePillToggle is a segmented control with keyboard navigation, not a chip.

Open with design

The chip frame and the component have drifted apart in two ways this PR doesn't settle:

  • two sizes vs our three. sm and xs are now both 24px and differ only in font size. 36 call sites chose between them for reasons that need reading one by one.
  • three styles vs our nine variants. His primary/secondary/outline don't cover success, warning, danger, info or muted, which the app needs.

And the one worth asking first: the frame says chips "only display information and have no further interaction", yet tags here are clickable, selectable and removable. That decides whether onClick, onRemove and selected belong on this component at all.

The real consolidation, for a follow-up

35 components still use the legacy .chip class rather than the primitive:

BetaFlag              'chip chip--xs … bg-primary900'  + an IonIcon
IntegrationSelect     'chip--xs cursor-pointer chip'
FeatureHistory        'chip chip--xs px-2'
ConnectedGroupSelect, EditPermissions, ChipInput, …

Each hand-rolls a pill with raw classes against a stylesheet carrying its own .dark block. Migrating them in batches and then deleting the legacy half of _chip.scss is its own piece of work.

How did you test this code?

  • npm run test:unit — 732 pass, 53 suites
  • npx rspack build — clean; tsc alone missed a broken import in project-components.js, so the bundle is the check that matters here
  • Storybook: Chip has a Selected story covering what ToggleChip's used to, plus new Tag and TagColourPicker stories
  • Checked the tag dropdown and applied tags in both themes against prod

🤖 Generated with Claude Code

talissoncosta and others added 9 commits September 25, 2026 13:39
@talissoncosta @claude
feat(chip): add semantic variants, a solid fill and a status dot
085e1a8 
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>
@talissoncosta @claude
refactor(experiments): rebuild StatusBadge on the chip primitive
898b2ef 
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>
@talissoncosta @claude
feat(tags): add the tag colour scale
cb4dd1b 
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>
@talissoncosta @claude
feat(icons): add stale and pr-dequeued, and colour pr-draft
d811952 
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>
@talissoncosta @claude
fix(tags): carry tag state in an icon, not the fill
e1dab25 
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>
@talissoncosta @claude
docs(chip): story the variants and how a tag composes Chip
7c37800 
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>
@talissoncosta @claude
feat(tags): take the tag palette from the design system Content colours
f2c5a55 
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>
@talissoncosta @claude
fix(tags): give the colour picker a shared component and a real swatch
3496878 
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>
@talissoncosta @claude
docs(tags): story Tag and the colour picker
a6bace8 
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>
@vercel

vercel Bot commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
flagsmith-frontend-preview Ready Ready Preview Sep 25, 2026 8:33pm UTC
flagsmith-frontend-staging Ready Ready Preview Sep 25, 2026 8:33pm UTC
1 Skipped Deployment
Project Deployment Actions Updated
docs Ignored Ignored Preview Sep 25, 2026 8:33pm UTC

Request Review

@coderabbitai

coderabbitai Bot commented Sep 25, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added front-end Issue related to the React Front End Dashboard refactor labels Sep 25, 2026
@talissoncosta
talissoncosta force-pushed the refactor/chip-selected-prop branch from 5caf0fb to 9d2dc6a Compare September 25, 2026 19:47
@talissoncosta
talissoncosta force-pushed the refactor/chip-selected-prop branch from 9d2dc6a to 766d04c Compare September 25, 2026 19:50
@talissoncosta
talissoncosta force-pushed the refactor/chip-selected-prop branch from 766d04c to 7407574 Compare September 25, 2026 19:53
@github-actions github-actions Bot removed the refactor label Sep 25, 2026
talissoncosta and others added 2 commits September 25, 2026 17:14
@talissoncosta @claude
refactor(chip): fold ToggleChip into a selected prop
5b31320 
ToggleChip was forty lines wrapping Chip to add one thing, a tick when
active, plus a conditional that existed only for the tag colour picker: with
no label, skip the box and let the tick mark the choice. That is a prop.

Chip gains `selected`, and `children` becomes optional, since a selected chip
with no children is exactly the bare swatch that conditional was for.

Tag loses its branch. It chose between ToggleChip and Chip on
`!hideNames && !!onClick`, so a clickable tag without a name could not show
selection at all. Now one Chip either way, with selection offered under the
same condition as before.

That branch was also skipping the feature_health guard that sits below it, so
a clickable unhealthy tag rendered even with the flag off. It no longer can.

The `!!tag.label &&` guard in the toggle path went too: TagContent already
returns null without a label, so both paths rendered the same thing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@talissoncosta @claude
refactor(tags): move VCSProviderTag out of the tags folder
18bd891 
It is not a tag. It does not use Tag or Chip, it renders its own markup with
a count and a provider icon, and it sits in tags/ by name alone. Its three
callers are split between feature-summary and feature-page, so it goes to the
top level with the other shared components rather than into either.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@talissoncosta
talissoncosta force-pushed the refactor/chip-selected-prop branch from 7407574 to 18bd891 Compare September 25, 2026 20:14
@talissoncosta @claude
fix(chip): take the default size from the chip frame
11fbc13 
The chip frame in Figma gives a 32px default with an 8px gap, and two sizes
rather than three. Ours was ~29px with a 4px gap, and Tag pinned itself to the
smallest size, which is why tags read as undersized next to everything around
them.

    .ds-chip    height 32px, gap 8px   (was ~29px, gap 4px)
    Tag         default                (was size='xs', 24px)

Height is stated rather than left to derive from padding, so a chip holding
only an icon matches one holding text. --sm gains an explicit height for the
same reason: with the base fixed it would otherwise have inherited 32px, and
its seventeen call sites would have grown.

The frame also shows two sizes where we have three, and three styles
(primary, secondary, outline) where we have nine variants. Neither is settled
here: `sm` and `xs` now differ only in font size, and the status variants the
app needs are not in the frame at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@talissoncosta @claude
fix(tags): show a checkbox only where a tag is actually selectable
33a6f41 
Tag inferred selection from `!hideNames && !!onClick`, which conflates two
different lists. The tag dropdown is a multi-select and wants a checkbox. The
tags already applied to a feature are a display list where clicking removes,
and they were rendering an unchecked box against tags that are applied, saying
the opposite of what is true.

It now shows a checkbox when `selected` is passed and not otherwise, so the
caller says which list it is. AddEditTags passes it in the dropdown;
TagValues does not.

That leaves hideNames reading nothing, so it goes, along with the two callers
forwarding it. It only ever existed to pick the branch.

The unchecked box is not new, prod does the same, but the 32px chip made it
hard to miss.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@talissoncosta @claude
fix(tags): match any tag colour to a swatch by hue
95b521c 
The lookup table only held the twenty colours the picker has offered, so
a tag storing anything else fell through to a neutral fill and stopped
reading as a tag. `color` is a bare CharField with no validator, so the
API and tag imports can store any string.

Matching on hue instead answers for every colour and drops the table.
Nineteen of the twenty resolve exactly as before; #5d6d7e, a desaturated
slate, now takes blue rather than grey, which is the hue it actually has.

Contrast is still settled by the palette rather than by the stored
colour: the eleven Content swatches stay the only fills a tag can take,
and the match only chooses between them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@talissoncosta
talissoncosta force-pushed the fix/tags-accessibility-8465 branch from a6bace8 to f232f9f Compare September 29, 2026 11:17
@talissoncosta

talissoncosta commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the three-PR stack that replaced #8569:

I checked the four commits here against the stack before closing. Three were already in it:

Only 18bd8910e (move VCSProviderTag out of the tags folder) was missing, and it is now on #8614.

Retargeting this one would not have helped: pointed at the new base it still showed 41 files across 14 commits, because its history predates the rewrite and the shared work has different SHAs.

This branch was successfully deployed

2 active deployments
Preview – flagsmith-frontend-preview — 95b521c7 Deployed Sep 25, 2026 by vercel[bot]
View deployment
Preview – flagsmith-frontend-staging — 95b521c7 Deployed Sep 25, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

front-end Issue related to the React Front End Dashboard refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@talissoncosta