Repository navigation
fix(tags): make tags and their picker accessible - #8613
talissoncosta wants to merge 73 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe frontend adds a content-colour palette with generated TypeScript mappings, CSS variables, tag utilities, and contrast checks. Tag rendering now uses shared chips, swatch utilities, and system-tag icons. A shared picker is used for tag colour selection. Tag management and filtering update row interactions, creation checks, and search behaviour. Storybook coverage and end-to-end selectors are updated. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Three tag-picker problems should be fixed before merge. A feature with no tags selected may not allow tags to be added. Saving an edit to a tag can silently add it to or remove it from the feature. Keyboard users cannot reach Edit or Delete from the new action menu. A smaller issue is that some unusual stored colour values can break tag rendering. 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
front-end
Docker builds report
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21044 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #21044 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21044 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21042 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21044 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21042 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21043 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21042 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21042 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21043 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21033 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21033 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Eleven tints from the design system's Content palette, plus the ink their labels take. A tag is the same on both themes, fill and dark label, so there is no dark counterpart and the utility reads the primitives directly. The fill sits at 1.18:1 against white, under the 3:1 for non-text. That is the design as handed off and it holds: a tag is read from its label, which clears AA on every tint, not from its edge. The test pins that, and that no two tints collide, since tags are told apart by colour alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The API already creates a dequeued-PR tag; the UI rendered it without an icon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tags colour a chip from a class, because the colour is a decorative one a user picked rather than a semantic role. A variant that emits no utilities keeps that class from having to beat the defaults on source order alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tags rendered the stored colour as text on an 8% tint of itself, so contrast was whatever the hue gave and most of the palette failed AA. The maths was copied to four places, two of them inline style strings behind dangerouslySetInnerHTML. A tag now takes the nearest Content tint, matched on OKLCH hue, so no tag changes identity and nothing migrates. `color` is an unvalidated CharField, so matching rather than a lookup table is what makes every tag land on an accessible pair. System tags carry their state in a coloured icon on plain surface, so it survives for anyone who cannot tell fills apart. Two fixes fall out. `Tag` took `disabled` from the plan the account is on, which is the caller's to know and pulled the whole utils module into anything drawing a tag. And the lock on a permanent tag comes back: it has been dead since #4822 dropped the last argument from the renderIcon call, leaving `isPermanent` undefined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The picker drew its options as tags with a label, so you chose a colour by reading the word on it. Each option is now the colour itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
00fd156 to
0eab747
Compare
0722800 to
437cb11
Compare
sort-keys-fix had put 2xl first and xl last, so the scale read in no order at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It promised "a menu, a usage count" while the CSS animates to a fixed 20px, so anything wider than the menu button is clipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The edit effect ran on editSuccess but asked createSuccess, so saving an edit never called onComplete and the form stayed open on a tag that had already saved. save() also carried its own copy of the disabled condition, without the duplicate-name check the button has, so Enter created the tag the form was refusing. One condition now serves both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It lowercased the label and then compared the raw search term, so ONB found no Onboarding. Same bug as the tag panel's, in the filter the features list, the feature page and Import all use. Drops a filter((tag) => tag) that allocated a new array per render. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"archived" and "untagged" ignored the search box, so a term matching nothing showed "No tags" with two rows underneath it. They filter on their own labels now, and the message waits until nothing is left. Names the untagged id, which was written out as '' four times. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 16px circle of one colour is what ColorSwatch is, and it is decorative in both, so aria-hidden comes with it. Tag.scss held nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The disabled note carried its own decision record, and the Partial note sat on onClick while explaining tag, half of it describing a create row that no longer previews one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Giving the base a fixed height left --xs inheriting 32px, nearly double what its padding used to produce, across nine components outside tags. Stated now, as --sm already was. onKeyDown replaced the built-in activation instead of running beside it, so a chip with a caller's handler could not be activated by Enter or Space. SdkPicker is the one that has one. onClick and onRemove are now mutually exclusive in the type: a button inside a button reaches a screen reader as neither. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Against main the only change to --xs is now height: auto, so it renders exactly as it did rather than at a number I worked back from the padding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tag is a display chip in a feature row and a control in the filter, so placing it by role forces a choice the component does not make. Same shape as Components/Charts and Components/Diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
createTag waited on .tag--select, a class that went with the old picker, and on an "Add New Tag" button that is a row now. The first timed out at 20s and failed the run on every retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Pass false when no tags are selected. · AddEditTags.tsx:193
frontend/web/components/tags/AddEditTags/AddEditTags.tsx:193
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
falsewhen no tags are selected.
valueis optional. Whenvalueis undefined, this expression passeschecked={undefined}. The newTagRowthen setsisCheckboxto false and disables its selection button. Users cannot select an existing tag from this initial state.Proposed fix
- checked={value?.includes(tag.id)} + checked={value?.includes(tag.id) ?? false}
🟠 Major · Preserve tag selection after editing. · AddEditTags.tsx:278
frontend/web/components/tags/AddEditTags/AddEditTags.tsx:278
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve tag selection after editing.
The corrected edit-success effect in
frontend/web/components/tags/CreateEditTag.tsx, Line 75, now invokes this callback.selectTagtoggles membership. Saving an already selected tag therefore removes it from the feature. Saving an unselected tag adds it.Return to the selection view without calling
selectTagafter an edit. Keep automatic selection for newly created tags.Proposed fix
- onComplete={(tag: TTag) => { - selectTag(tag) + onComplete={() => { setTab('SELECT') }}
🟠 Major · Make the tag actions keyboard-operable. · DropdownMenu.tsx:80
frontend/web/components/base/DropdownMenu.tsx:80
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the tag actions keyboard-operable.
The new tag rows use this menu for Edit and Delete. Its action elements are
divelements with onlyonClick. They have notabIndexor keyboard handler. Keyboard users can open the menu but cannot focus or activate either action.Use focusable action buttons. Move focus into the menu when it opens, and support closing it with Escape and restoring trigger focus. If this component uses menu semantics, implement the corresponding menu roles and keyboard behaviour. (w3.org)
Based on learnings: non-native clickable controls require keyboard activation equivalent to their click handler.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 0c506c81-2499-4dec-84db-7408fc5db3c4
📒 Files selected for processing (36)
frontend/common/theme/tokens.tsfrontend/common/useContainClicks.tsfrontend/documentation/components/Chip.stories.tsxfrontend/documentation/components/ColorSwatch.stories.tsxfrontend/documentation/components/Icons.stories.tsxfrontend/documentation/components/Tag.stories.tsxfrontend/documentation/components/TagColourPicker.stories.tsxfrontend/documentation/components/VCSProviderTag.stories.tsxfrontend/e2e/helpers/e2e-helpers.playwright.tsfrontend/scripts/generate-tokens.mjsfrontend/web/components/ColorSwatch.tsxfrontend/web/components/base/Chip/Chip.scssfrontend/web/components/base/Chip/Chip.tsxfrontend/web/components/base/DropdownMenu.tsxfrontend/web/components/feature-summary/FeatureAction.tsxfrontend/web/components/icons/Icon.tsxfrontend/web/components/pages/environment-settings/EnvironmentSettingsPage.tsxfrontend/web/components/tables/TableTagFilter.tsxfrontend/web/components/tags/AddEditTags/AddEditTags.scssfrontend/web/components/tags/AddEditTags/AddEditTags.tsxfrontend/web/components/tags/ColourSelect/ColourSelect.scssfrontend/web/components/tags/ColourSelect/ColourSelect.tsxfrontend/web/components/tags/ColourSelect/index.tsfrontend/web/components/tags/CreateEditTag.tsxfrontend/web/components/tags/Tag/Tag.tsxfrontend/web/components/tags/TagColourPicker/TagColourPicker.scssfrontend/web/components/tags/TagColourPicker/TagColourPicker.tsxfrontend/web/components/tags/TagContent.tsxfrontend/web/components/tags/TagFilter.tsxfrontend/web/components/tags/TagRow/TagRow.scssfrontend/web/components/tags/TagRow/TagRow.tsxfrontend/web/components/tags/utils/__tests__/tagSwatch.test.tsfrontend/web/components/tags/utils/index.tsfrontend/web/components/tags/utils/systemTag.tsfrontend/web/components/tags/utils/tagChipHtml.tsfrontend/web/components/tags/utils/tagSwatch.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
A tag whose stored colour is "constructor" or "__proto__" found an inherited property, so getTagSwatch handed back something that is not a swatch: a garbage utility class, and a throw in swatchLabel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
as const told the compiler what the pair is; a return type has it check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@themis-blindfold review |
![themis-blindfold[bot]](https://avatars.githubusercontent.com/in/4278417?s=60&v=4){kind=small}
⚖️ Themis review: 🟠 Fix before mergeCaptured checks passed, including unit tests and linting. The compact tag filter now has a selectable Stale tag path without the plan-entitlement guard, so this accessibility refactor should not merge until that regression is fixed.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a meaningful accessibility improvement for tag discovery and editing. The entitlement regression is narrow but lets a non-entitled user activate a plan-gated filter, so it needs correction before release. 🧭 Assumptions & unverified claimsFocused local tests could not be rerun because frontend dependencies are unavailable in this checkout. A much friendlier tag picker, once the Stale gate is put back on its hinge · reviewed at a6df67d |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Tag used to read Utils.tagDisabled itself; moving that to a prop left this filter passing none, so an organisation without STALE_FLAGS could select the Stale tag here. The other clickable call sites render pseudo-tags with no type, which tagDisabled never locks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@themis-blindfold review |
⚖️ Themis review: 🟠 Fix before mergeCI has passed, and the tag colour and selection changes are coherent. One accessibility gap remains: the new per-tag Edit/Delete actions are mouse-only, so keyboard users cannot manage custom tags.
🟠 Majors
⚖️ Acknowledged
📝 Walkthrough
🧪 How to verify
Product take: This is a solid accessibility improvement for tag selection and colour choice. 🧭 Assumptions & unverified claimsThe checked-out The swatches can speak now; the action menu still needs a keyboard · reviewed at cb455c8 |
|
Closing in favour of a stack, after feedback that this was too big to review. 73 commits and 57 files became five PRs, each reviewable on its own:
#8689 carries the accessibility fix this was opened for: the colour signal off the fill, onto a neutral border with a coloured icon. Parked, not dropped. The colour picker and the tag panel are not in the stack. Their container is one of the eleven Two things worth carrying over:
Please leave #8465 stays open until the parked half lands. |
This branch was successfully deployed
4 participants
docs/if required so people know about the feature.Changes
Closes #8465. Replaces #8569 and #8614.
Tags drew the stored colour as text on an 8% tint of itself, so most of the palette failed AA. The
picker was worse: swatches with no accessible name, a selected state only a sighted user could see,
and a checkbox drawn inside the chip it was selecting.
Colour. Custom tags take one of eleven Content tints, and every colour a tag can hold maps onto
one, so nothing migrates. System tags keep the plain surface and carry their state in an icon. Same
fill on both themes, per Dragos's frame.
Picker. A row owns the selection, with
role="checkbox",aria-checkedand arrow keys; per-tagactions move into a menu.
ToggleChipbecomes aselectedprop onChipand goes. The swatch gridis a
ColorSwatchin a button, named by its hue. A plan-locked tag is focusable now, so arrow keyspass it and its tooltip can be read.
Creating a tag. The "Add New Tag" button becomes one row under the list: named it creates the
tag, unnamed it opens the full form.
Pre-existing bugs fixed on the way:
editSuccessbut askedcreateSuccess.renderIcon.DropdownMenuportals to the body, so picking Edit opened the tag form and shut it again.Known and deliberate:
clears AA on every tint, not from its edge.
TableTagFilter's dot still draws the stored colour, so an older tag shows a saturated dot besidea pale chip. A 16px circle in a chip tint is close to invisible, so it needs a design answer.
text rather than as a chip.
Not here: the Chip variants (#8612) and the ~60 legacy
.chipsites.How did you test this code?
tagSwatches.test.tsasserts every label clears 4.5:1 and that no two tints collide;tagSwatch.test.tspins every colour a tag can hold to its swatch. Unit suite and E2E pass,tscunchanged against main. Storybook in both themes, under Components → Tags.
To check manually, in a project with several coloured tags:
open, and close on save.