Repository navigation
refactor(tags): rebuild the tag picker on a row - #8614
talissoncosta wants to merge 4 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. 📝 WalkthroughWalkthroughChip now supports selected and unselected presentation, and the ToggleChip component has been removed. The changes add TagRow and update tag rendering, filtering, and management. DropdownMenu stops native mouseup and touchend events from propagating while open. Several components also use updated VCSProviderTag import paths. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The rebuilt tag picker can change tags in read-only mode, may prevent selecting a first tag, can show the system health tag when that feature is off, and has keyboard and screen-reader gaps in the new row and menu. These should be fixed before merging. 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
|
❌ private-cloud · depot-ubuntu-latest-16 — run #20918 (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❌ oss · depot-ubuntu-latest-16 — run #20918 (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 #20917 (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 #20917 (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 #20917 (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-arm-16 — run #20917 (attempt 1)Playwright Test Results (oss - 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 |
Visual Regression18 screenshots compared. See report for details. |
ToggleChip was a chip with a checkbox drawn inside it, and its only consumer was Tag. Selection is a state a chip can be in, not a second component, so it becomes a prop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The picker drew each tag as a chip with a checkbox inside it and the actions beside it, so selection and the tag itself were the same object. A row now owns the selection, as a checkmark on the right where the five other lists in the app put it, and the actions collapse into a menu that appears on hover or focus. Tag and TagContent go back to drawing and nothing else: no permission lookups, no plan checks, no colour maths. 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, the InlineModal holding the tag list, say, counted a click on a menu item as one and closed: picking Edit opened the form and shut it again a moment later, because that watcher defers its close by 100ms. The event now stops at the menu's root. A native listener, 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>
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>
2486d4c to
5371782
Compare
0722800 to
437cb11
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 52e98e72-7e66-48a8-9e40-bd41aab1300b
📒 Files selected for processing (25)
frontend/documentation/components/Chip.stories.tsxfrontend/documentation/components/TagRow.stories.tsxfrontend/documentation/components/ToggleChip.stories.tsxfrontend/documentation/components/VCSProviderTag.stories.tsxfrontend/web/components/ToggleChip.scssfrontend/web/components/ToggleChip.tsxfrontend/web/components/VCSProviderTag.tsxfrontend/web/components/base/Chip/Chip.scssfrontend/web/components/base/Chip/Chip.tsxfrontend/web/components/base/DropdownMenu.tsxfrontend/web/components/feature-page/FeatureNavTab/CodeReferences/components/RepoSectionHeader.tsxfrontend/web/components/feature-summary/FeatureTags.tsxfrontend/web/components/feature-summary/ProjectFeatureRow.tsxfrontend/web/components/tables/TableTagFilter.tsxfrontend/web/components/tags/AddEditTags.tsxfrontend/web/components/tags/Tag.tsxfrontend/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/TagRow/index.tsfrontend/web/components/tags/TagValues.tsxfrontend/web/project/project-components.jsfrontend/web/styles/project/_tags.scss
💤 Files with no reviewable changes (4)
- frontend/web/components/ToggleChip.tsx
- frontend/web/project/project-components.js
- frontend/web/components/ToggleChip.scss
- frontend/documentation/components/ToggleChip.stories.tsx
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| padding: 4px 10px; | ||
| // 32px from the design system's chip frame. Stated, so an icon-only chip | ||
| // matches one holding text. | ||
| height: 32px; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Give the xs variant its own height.
The new 32px base height also applies to ds-chip--xs. The xs variant has no height override, so extra-small chips render as tall as default chips. Set an explicit xs height alongside its existing size rules.
| role={role ?? (onClick ? 'button' : undefined)} | ||
| tabIndex={interactive ? tabIndex ?? 0 : undefined} | ||
| aria-checked={ariaChecked} | ||
| aria-checked={ariaChecked ?? selected} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expose the selected state with the Chip’s interactive role.
In TagFilter, a clickable Tag gives its Chip role button. Line 80 then puts aria-checked on that button, although a toggle button uses aria-pressed. Screen-reader users cannot reliably determine which filter chips are selected. Use aria-pressed for button chips, and retain aria-checked for radio chips. (w3.org)
| selected={value?.includes(tag.id)} | ||
| tag={tag} | ||
| <TagRow | ||
| checked={value?.includes(tag.id)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass a defined checked state for an empty selection.
When value is omitted, value?.includes(tag.id) is undefined. TagRow then sets selectable to false despite receiving onToggle, so users cannot select the first tag. Pass checked={!!value?.includes(tag.id)}.
| checked={value?.includes(tag.id)} | ||
| disabled={Utils.tagDisabled(tag)} | ||
| key={tag.id} | ||
| onToggle={selectTag} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Disable row selection in read-only mode.
When readOnly is true and value is defined, each TagRow still receives selectTag. A click or keyboard activation therefore calls onChange and changes the selection. Omit onToggle when readOnly is true.
| !readOnly && | ||
| !!createEditTagPermission && | ||
| !tag.is_system_tag && ( | ||
| <DropdownMenu |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the new Edit and Delete menu keyboard-operable.
The DropdownMenu added here renders its Edit and Delete items as clickable div elements with no focus or key handler. A keyboard user can reach the menu trigger but cannot activate either item. Use keyboard-operable menu items before replacing the former actions with this menu. Based on learnings: non-native interactive elements need keyboard activation.
Source: Learnings
| )} | ||
| onClick={disabled || !onClick ? undefined : () => onClick(tag as TTag)} | ||
| size='xs' | ||
| onClick={disabled || !onClick ? undefined : () => onClick(tag)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the feature-health guard in every Tag list.
Tag now renders an UNHEALTHY tag without checking feature_health. TagValues and TableTagFilter filter that tag, but TagFilter maps the unfiltered query results to clickable Tags. With the feature disabled, users can now see and select the system health tag in TagFilter. Filter it in TagFilter before rendering, as the other lists do.
| const next = e.currentTarget[step] | ||
| if (next instanceof HTMLElement) { | ||
| e.preventDefault() | ||
| next.focus() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip rows that cannot receive arrow-key focus.
If a disabled row lies between two selectable rows, it has no tabIndex. ArrowDown selects that immediate sibling, prevents the normal key action and calls focus() on the non-focusable row. Focus remains on the original row, so another ArrowDown cannot reach the following selectable row. Find the next focusable TagRow before preventing the key action.
| if (!selectable) return | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| toggle() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle activation keys only when the row has focus.
When the trailing menu button has focus, its Enter or Space keydown bubbles to this handler. The handler toggles the tag and calls preventDefault(), which can prevent the button’s keyboard click from opening the menu. Return when e.target !== e.currentTarget before handling row activation or arrow movement. React propagates child events to parent handlers, and preventDefault() cancels default behaviour. (react.dev)
The colour picker draws eleven tags with no text, so each reached a screen reader as an unnamed button. A tag with no label is now named by its colour, "light-green" as "Light green", in Tag rather than in the picker, so any label-less clickable tag is covered. `active` drove only the tick, leaving selected and unselected indistinguishable to assistive technology. Chip takes aria-pressed and ToggleChip passes it, but only where it toggles: announcing a pressed state on a chip that does nothing would be a lie. Also takes the colour picker's onClick from #8614, so the two branches stop diverging on that line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Folded into #8613. Splitting here cost more than it bought: #8613 added 61 lines to Both rebase conflicts were modify-vs-delete, which is the same thing showing up in git. #8613 is now 46 files, and the accessibility work landed on |
This branch was successfully deployed
2 participants
docs/if required so people know about the feature.Changes
Contributes to #8465. Stacked on #8613, which has to land first: the row draws tags in their new
colours.
The tag picker drew each tag as a chip with a checkbox inside it and the actions beside it, so the
selection control and the tag itself were the same object. There was no
roleoraria-checkedon any of it either, so the checkbox was decoration.
TagRowowns the selection, as a checkmark on the right where the five other lists in the appput it, with
role="checkbox",aria-checkedand arrow-key navigation.between the tag and its checkbox.
ToggleChipwas a chip with a checkbox drawn inside it and its only consumer wasTag.Selection is a state a chip can be in, not a second component, so it becomes a
selectedpropand
ToggleChipgoes.TagandTagContentgo back to drawing and nothing else: no permission lookups, no planchecks, no colour maths.
A menu inside a panel closed the panel
DropdownMenudraws through a portal on the body, so it is not a descendant of whatever openedit. Anything watching for a click outside itself counted a click on a menu item as one: picking
Edit on a tag opened the form and shut it again a moment later, because that watcher defers its
close by 100ms. The event now stops at the menu's root.
That is a general fix, not a tags one. It only shows up when a portalled menu sits inside
something with outside-click dismissal, which this PR is the first to do.
Chip picks up its frame height
.ds-chipderived its height from the inherited line-height, so a chip holding only an icon wasshorter than one holding text and the row could not line anything up. It now states the 32px and
24px the design system's chip frame fixes.
That is a piece of frame conformance arriving in a picker PR, which is not ideal. The rest of it
(three styles rather than seven variants, the paddings, the 12px type) is deferred until the new
colour system lands: see #8612. This much is here because the row needs it to sit right.
How did you test this code?
Storybook in both themes for
TagRowandChip, and manually in the tag panel. Full unit suitepasses.
To check manually, in the tag panel on a feature: