feat: add accessible toolbar component #1545
Conversation
WalkthroughIntroduces a new UI Toolbar component with Root, Button, Link, and Separator fragments, associated context, styles, Storybook story, and tests. Adds toolbar styles to the default theme and registers "Toolbar" in the release components list. Implements roving focus navigation and orientation-aware separator behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant Toolbar.Root
participant RovingFocusGroup
participant Context as ToolbarRootContext
participant Item as Toolbar.Button/Link/Separator
App->>Toolbar.Root: Render with props (orientation, loop, dir)
Toolbar.Root->>Context: Provide { rootClass, orientation }
Toolbar.Root->>RovingFocusGroup: Initialize roving focus (dir, loop)
loop For each child
App->>Item: Render child
Item->>Context: Consume { rootClass, orientation }
Item->>Item: Compose className (e.g., rootClass-*)
end
User->>RovingFocusGroup: Arrow keys
RovingFocusGroup-->>Item: Move focus to next/prev focusable
Note over Item,Context: Separator defaults to opposite of root orientation unless overridden
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
CoverageThis report compares the PR with the base branch. “Δ” shows how the PR affects each metric.
Coverage decreased for at least one metric. Please add or update tests to improve coverage. Run |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/components/ui/Toolbar/tests/Toolbar.behavior.test.tsx (1)
8-28: Good keyboard-nav coverage; consider adding vertical orientation and Home/End tests.Expand to assert Up/Down behavior when orientation="vertical" and Home/End jump to first/last item. This will harden the roving focus contract.
Apply this diff to add a vertical test:
@@ describe('Toolbar keyboard navigation and a11y', () => { test('arrow keys move focus between items', async () => { @@ }); + test('vertical orientation uses ArrowDown/ArrowUp', async () => { + render( + <Toolbar.Root aria-label="Editor toolbar" orientation="vertical"> + <Toolbar.Button>Bold</Toolbar.Button> + <Toolbar.Button>Italic</Toolbar.Button> + <Toolbar.Link href="#">Link</Toolbar.Link> + </Toolbar.Root> + ); + const user = keyboard(); + await user.tab(); + const buttons = screen.getAllByRole('button'); + const link = screen.getByRole('link'); + expect(buttons[0]).toHaveFocus(); + await user.keyboard('{ArrowDown}'); + expect(buttons[1]).toHaveFocus(); + await user.keyboard('{ArrowDown}'); + expect(link).toHaveFocus(); + await user.keyboard('{ArrowUp}'); + expect(buttons[1]).toHaveFocus(); + });src/components/ui/Toolbar/context/ToolbarRootContext.tsx (1)
1-10: Optional DX helper: export a safe hook for the context.Avoids repeating null checks at call sites and standardizes the error message.
Apply this diff:
- import { createContext } from 'react'; + import { createContext, useContext } from 'react'; @@ const ToolbarRootContext = createContext<ToolbarRootContextType>(null); export default ToolbarRootContext; + +export const useToolbarRootContext = () => { + const ctx = useContext(ToolbarRootContext); + if (!ctx) throw new Error('Toolbar subcomponents must be used within Toolbar.Root'); + return ctx; +};src/components/ui/Toolbar/fragments/ToolbarLink.tsx (1)
18-19: Consider passing customRootClass instead of only className.If Link composes its own base class, using customRootClass={
${rootClass}-link} ensures toolbar-specific styling wins without specificity battles.Apply this diff:
- <Link ref={ref} className={clsx(`${rootClass}-link`, className)} {...props} /> + <Link + ref={ref} + customRootClass={`${rootClass}-link`} + className={className} + {...props} + />src/components/ui/Toolbar/stories/Toolbar.stories.tsx (1)
11-22: Let Template accept args and add a vertical story variant.Helps visualize orientation styles and behavior in Storybook.
Apply this diff:
-const Template = (_args: any) => { +const Template = (args: any) => { return ( <SandboxEditor className="space-y-4 pt-4"> - <Toolbar.Root aria-label="Formatting options"> + <Toolbar.Root aria-label="Formatting options" {...args}> <Toolbar.Button>Bold</Toolbar.Button> <Toolbar.Button>Italic</Toolbar.Button> <Toolbar.Separator /> <Toolbar.Link href="#">Link</Toolbar.Link> </Toolbar.Root> </SandboxEditor> ); }; @@ export const Default = { args: {} }; + +export const Vertical = { + args: { orientation: 'vertical' } +};styles/themes/components/toolbar.scss (1)
10-26: Optional: add disabled styling for better affordance.Helps convey non-interactivity in both themes.
Apply this diff:
.rad-ui-toolbar-button, .rad-ui-toolbar-link { @@ cursor: pointer; + &:disabled, + &[aria-disabled='true'] { + opacity: 0.5; + cursor: not-allowed; + pointer-events: none; + }src/components/ui/Toolbar/fragments/ToolbarButton.tsx (1)
16-20: Ensure there’s only one tabbable element per item.If RovingFocusGroup.Item itself becomes focusable, make the inner Button non-tabbable (or use an “asChild” pattern) to avoid double tab stops.
Would you like me to check RovingFocusGroup.Item’s API and propose a concrete change (e.g., asChild or tabIndex management)?
src/components/ui/Toolbar/fragments/ToolbarSeparator.tsx (2)
3-3: Use default import for clsx to avoid interop pitfalls.
clsx’s primary export is default; named import can break depending on bundler/tsconfig. Prefer default import.-import { clsx } from 'clsx'; +import clsx from 'clsx';
11-13: Context guard: keep hard error but consider dev-only throw.The invariant is good. If you want zero runtime throws in prod, gate the error behind
process.env.NODE_ENV !== 'production'and returnnullin prod.src/components/ui/Toolbar/fragments/ToolbarRoot.tsx (2)
3-3: Prefer default import for clsx for consistency with ecosystem.Same rationale as in Separator.
-import { clsx } from 'clsx'; +import clsx from 'clsx';
30-35: Optional: dev-only warning when missing accessible name.Toolbars should have an accessible name via
aria-labeloraria-labelledby. Consider a dev-only warning if neither is provided.src/components/ui/Toolbar/Toolbar.tsx (2)
17-20: Gate console warning to development.Avoid noisy logs in production.
-const Toolbar = React.forwardRef<ToolbarElement, ToolbarProps>((_props, _ref) => { - console.warn('Direct usage of Toolbar is not supported. Please use Toolbar.Root, Toolbar.Button, etc. instead.'); +const Toolbar = React.forwardRef<ToolbarElement, ToolbarProps>((_props, _ref) => { + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn('Direct usage of Toolbar is not supported. Please use Toolbar.Root, Toolbar.Button, etc. instead.'); + } return null; }) as ToolbarComponent;
1-5: Re-export root props for DX (optional).Handy for consumers importing from the barrel.
import React from 'react'; import ToolbarRoot from './fragments/ToolbarRoot'; +export type { ToolbarRootProps } from './fragments/ToolbarRoot';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
scripts/RELEASED_COMPONENTS.cjs(1 hunks)src/components/ui/Toolbar/Toolbar.tsx(1 hunks)src/components/ui/Toolbar/context/ToolbarRootContext.tsx(1 hunks)src/components/ui/Toolbar/fragments/ToolbarButton.tsx(1 hunks)src/components/ui/Toolbar/fragments/ToolbarLink.tsx(1 hunks)src/components/ui/Toolbar/fragments/ToolbarRoot.tsx(1 hunks)src/components/ui/Toolbar/fragments/ToolbarSeparator.tsx(1 hunks)src/components/ui/Toolbar/stories/Toolbar.stories.tsx(1 hunks)src/components/ui/Toolbar/tests/Toolbar.behavior.test.tsx(1 hunks)styles/themes/components/toolbar.scss(1 hunks)styles/themes/default.scss(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/components/ui/Toolbar/fragments/ToolbarButton.tsx (1)
src/components/ui/Button/Button.tsx (1)
ButtonProps(15-20)
src/components/ui/Toolbar/tests/Toolbar.behavior.test.tsx (1)
test-utils/index.ts (2)
keyboard(64-64)axe(56-62)
src/components/ui/Toolbar/fragments/ToolbarSeparator.tsx (1)
src/components/ui/Separator/Separator.tsx (1)
SeparatorProps(12-17)
src/components/ui/Toolbar/fragments/ToolbarLink.tsx (1)
src/components/ui/Link/Link.tsx (1)
LinkProps(10-13)
src/components/ui/Toolbar/fragments/ToolbarRoot.tsx (1)
src/components/ui/Progress/Progress.tsx (1)
COMPONENT_NAME(8-8)
🪛 GitHub Actions: Lint
styles/themes/components/toolbar.scss
[error] 28-28: stylelint: no-descending-specificity: Expected selector '.rad-ui-toolbar-link' to come before selector '.rad-ui-toolbar .rad-ui-toolbar-link:focus-visible'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: coverage
- GitHub Check: build
🔇 Additional comments (5)
scripts/RELEASED_COMPONENTS.cjs (1)
27-28: Toolbar registered in release list — looks good.Placement after ToggleGroup is consistent with the existing ordering.
styles/themes/default.scss (1)
46-46: Theme import added correctly.The toolbar Sass module is now included in the default theme.
src/components/ui/Toolbar/tests/Toolbar.behavior.test.tsx (1)
30-40: Axe assertion is fine; prefer awaiting run completion on the container root.Current usage is correct. No changes required.
src/components/ui/Toolbar/fragments/ToolbarRoot.tsx (1)
36-51: Roving focus wiring looks solid.Provider + RovingFocusGroup composition is clean; memoized context avoids needless rerenders.
src/components/ui/Toolbar/Toolbar.tsx (1)
24-27: Static subcomponent wiring LGTM.Clear, conventional API surface.
| return ( | ||
| <RovingFocusGroup.Item role="link"> | ||
| <Link ref={ref} className={clsx(`${rootClass}-link`, className)} {...props} /> | ||
| </RovingFocusGroup.Item> |
There was a problem hiding this comment.
Remove duplicate interactive role to avoid nested roles.
RovingFocusGroup.Item wrapping a native already inside should not also expose role="link"; it risks duplicate semantics and confused focus/AT output.
Apply this diff:
- <RovingFocusGroup.Item role="link">
+ <RovingFocusGroup.Item>
<Link ref={ref} className={clsx(`${rootClass}-link`, className)} {...props} />
</RovingFocusGroup.Item>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <RovingFocusGroup.Item role="link"> | |
| <Link ref={ref} className={clsx(`${rootClass}-link`, className)} {...props} /> | |
| </RovingFocusGroup.Item> | |
| return ( | |
| <RovingFocusGroup.Item> | |
| <Link ref={ref} className={clsx(`${rootClass}-link`, className)} {...props} /> | |
| </RovingFocusGroup.Item> |
🤖 Prompt for AI Agents
In src/components/ui/Toolbar/fragments/ToolbarLink.tsx around lines 16 to 19,
the RovingFocusGroup.Item is given a duplicate interactive role ("link") while
the inner Link already renders a native anchor; remove the role prop from
RovingFocusGroup.Item to avoid nested/duplicated semantics and let the inner
Link/a provide the accessible role — simply delete the role="link" attribute (or
set it to undefined) on RovingFocusGroup.Item so focus handling remains but no
duplicate role is exposed.
| <div | ||
| ref={ref} | ||
| role="toolbar" | ||
| className={clsx(rootClass, className)} | ||
| {...dataAttributes} | ||
| {...props} | ||
| > | ||
| {children} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add required ARIA attributes to the toolbar container.
For vertical toolbars, aria-orientation should be set; also mirror dir on the DOM element for correct bidirectional behavior.
<div
ref={ref}
role="toolbar"
+ dir={dir}
+ aria-orientation={orientation}
className={clsx(rootClass, className)}
{...dataAttributes}
{...props}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div | |
| ref={ref} | |
| role="toolbar" | |
| className={clsx(rootClass, className)} | |
| {...dataAttributes} | |
| {...props} | |
| > | |
| {children} | |
| <div | |
| ref={ref} | |
| role="toolbar" | |
| dir={dir} | |
| aria-orientation={orientation} | |
| className={clsx(rootClass, className)} | |
| {...dataAttributes} | |
| {...props} | |
| > | |
| {children} |
🤖 Prompt for AI Agents
In src/components/ui/Toolbar/fragments/ToolbarRoot.tsx around lines 40 to 47,
the toolbar container is missing required ARIA and direction attributes: add
aria-orientation when the toolbar is vertical (e.g.,
aria-orientation={orientation === 'vertical' ? 'vertical' : undefined}) and set
the DOM dir attribute to mirror the current direction (e.g., dir={dir ||
undefined} or derive from context/props) so the element reflects bidi behavior;
ensure these attributes are passed on the root div alongside existing props and
do not override consumers' explicit props.
| <Separator | ||
| ref={ref} | ||
| decorative | ||
| orientation={separatorOrientation} | ||
| className={clsx(`${rootClass}-separator`, className)} | ||
| {...props} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent consumers from overriding decorative (enforce presentational separator).
Spreading ...props last allows decorative={false} to override the intended presentational behavior. Move the spread before fixed props to make decorative non-overridable.
<Separator
- ref={ref}
- decorative
- orientation={separatorOrientation}
- className={clsx(`${rootClass}-separator`, className)}
- {...props}
+ ref={ref}
+ {...props}
+ decorative
+ orientation={separatorOrientation}
+ className={clsx(`${rootClass}-separator`, className)}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Separator | |
| ref={ref} | |
| decorative | |
| orientation={separatorOrientation} | |
| className={clsx(`${rootClass}-separator`, className)} | |
| {...props} | |
| /> | |
| <Separator | |
| ref={ref} | |
| {...props} | |
| decorative | |
| orientation={separatorOrientation} | |
| className={clsx(`${rootClass}-separator`, className)} | |
| /> |
🤖 Prompt for AI Agents
In src/components/ui/Toolbar/fragments/ToolbarSeparator.tsx around lines 17 to
23, the component spreads {...props} after fixed props which allows callers to
override decorative; move the {...props} spread before the fixed props (ref,
decorative, orientation, className) so decorative remains enforced as
presentational and cannot be overridden by consumer-supplied props.
| .rad-ui-toolbar-link { | ||
| text-decoration: none; | ||
| } |
There was a problem hiding this comment.
Fix stylelint no-descending-specificity by inlining link rule.
Move link-specific text-decoration into the shared item rule so it appears before the :focus-visible selector.
Apply this diff:
.rad-ui-toolbar-button,
.rad-ui-toolbar-link {
all: unset;
display: inline-flex;
align-items: center;
justify-content: center;
padding: 4px 8px;
border-radius: 4px;
background-color: var(--rad-ui-color-gray-50);
color: var(--rad-ui-color-slate-900);
cursor: pointer;
+ /* Link-specific reset must come after `all: unset` to stick */
+ a& { text-decoration: none; }
&:focus-visible {
outline: 2px solid var(--rad-ui-color-accent-900);
outline-offset: 2px;
}
}
-
- .rad-ui-toolbar-link {
- text-decoration: none;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .rad-ui-toolbar-link { | |
| text-decoration: none; | |
| } | |
| .styles/themes/components/toolbar.scss | |
| .rad-ui-toolbar-button, | |
| .rad-ui-toolbar-link { | |
| all: unset; | |
| display: inline-flex; | |
| align-items: center; | |
| justify-content: center; | |
| padding: 4px 8px; | |
| border-radius: 4px; | |
| background-color: var(--rad-ui-color-gray-50); | |
| color: var(--rad-ui-color-slate-900); | |
| cursor: pointer; | |
| /* Link-specific reset must come after `all: unset` to stick */ | |
| a& { text-decoration: none; } | |
| &:focus-visible { | |
| outline: 2px solid var(--rad-ui-color-accent-900); | |
| outline-offset: 2px; | |
| } | |
| } | |
| // Removed redundant standalone link rule: | |
| // .rad-ui-toolbar-link { | |
| // text-decoration: none; | |
| // } |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 28-28: stylelint: no-descending-specificity: Expected selector '.rad-ui-toolbar-link' to come before selector '.rad-ui-toolbar .rad-ui-toolbar-link:focus-visible'.
🤖 Prompt for AI Agents
In styles/themes/components/toolbar.scss around lines 28 to 30, the
.rad-ui-toolbar-link rule sets text-decoration after more specific
:focus-visible selectors causing stylelint no-descending-specificity; move the
link-specific text-decoration declaration into the shared .rad-ui-toolbar-item
(or whatever shared item rule immediately precedes :focus-visible) so the
text-decoration appears before the :focus-visible selector (i.e., remove the
separate .rad-ui-toolbar-link block and add text-decoration: none; to the shared
item rule above).
1 participant
Summary
Testing
npm test src/components/ui/Toolbar/tests/Toolbar.behavior.test.tsxhttps://chatgpt.com/codex/tasks/task_e_68c1a1f5c9ec833197f2975633335285
Summary by CodeRabbit
New Features
Style
Chores