RovingFocusGroup + Refactor Tabs component to use RovingFocusGroup #876
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a comprehensive roving focus management system. New React components—namely Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant RFRoot as RovingFocusRoot (Provider)
participant RFGroup as RovingFocusGroup (Manager)
participant RFItem as RovingFocusItem (Focusable Element)
User->>RFItem: Triggers focus (click/keydown)
RFItem->>RFGroup: Registers/updating focus state
RFGroup->>RFRoot: Propagates context values (direction, loop)
RFRoot->>RFItem: Applies focus management properties
Possibly related PRs
Suggested labels
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(1 hunks)src/core/utils/RovingFocusGroup/index.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx
- src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx
- src/core/utils/RovingFocusGroup/index.tsx
🔇 Additional comments (1)
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (1)
1-4: LGTM! Clean import structure.The imports are well-organized and follow good practices with proper path aliasing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (2)
7-35: 🛠️ Refactor suggestionAdd accessibility documentation and keyboard interaction examples.
The story should demonstrate and document accessibility features:
- Keyboard navigation patterns
- ARIA attributes
- Focus management behavior
Here's a suggested implementation:
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + parameters: { + docs: { + description: { + component: ` + RovingFocusGroup implements the roving tabindex pattern for keyboard navigation. + + Keyboard Interactions: + - Arrow Left/Right: Navigate horizontally + - Arrow Up/Down: Navigate vertically + - Home: Move focus to first item + - End: Move focus to last item + + ARIA Attributes: + - role="group" on the root + - role="menuitem" on items + - aria-orientation on groups + ` + } + } + }, render: () => <>
37-42: 🛠️ Refactor suggestionExpand story variants to demonstrate different navigation patterns.
The current
Allvariant doesn't showcase different navigation patterns and configurations.Here's a suggested implementation:
-export const All = { +export const Horizontal = { + parameters: { + docs: { + description: { + story: 'Horizontal navigation using left/right arrow keys' + } + } + }, args: { - className: '' + orientation: 'horizontal', + loop: true } }; + +export const Vertical = { + parameters: { + docs: { + description: { + story: 'Vertical navigation using up/down arrow keys' + } + } + }, + args: { + orientation: 'vertical', + loop: true + } +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(1 hunks)src/core/utils/RovingFocusGroup/index.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/core/utils/RovingFocusGroup/index.tsx (1)
5-9: LGTM! Clean compound component pattern implementation.The utility object follows the React compound component pattern, making the API clean and intuitive to use.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
31-31: Remove console.log statementRemove debug console.log statements before merging to production.
- console.log('Tab');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/Badge/Badge.tsx(0 hunks)src/components/ui/Badge/fragments/BadgeRoot.tsx(0 hunks)src/components/ui/Badge/stories/Badge.stories.tsx(2 hunks)src/core/utils/RovingFocusGroup/context/RovingFocuGroupContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/ui/Badge/fragments/BadgeRoot.tsx
- src/components/ui/Badge/Badge.tsx
✅ Files skipped from review due to trivial changes (2)
- src/core/utils/RovingFocusGroup/context/RovingFocuGroupContext.tsx
- src/components/ui/Badge/stories/Badge.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (1)
6-16: Implement core focus management functionality.As a roving focus group component, this implementation is missing essential features:
- Keyboard navigation handlers (arrow keys)
- Focus management logic
- ARIA attributes for accessibility
- TypeScript types for orientation and direction
Here's a suggested implementation:
-const RovingFocusGroup = ({ children, ...props }: { children: React.ReactNode }) => { +interface RovingFocusGroupProps { + children: React.ReactNode; + orientation?: 'horizontal' | 'vertical'; + loop?: boolean; + dir?: 'ltr' | 'rtl'; +} + +const RovingFocusGroup = ({ + children, + orientation = 'horizontal', + loop = true, + dir = 'ltr', + ...props +}: RovingFocusGroupProps) => { + const handleKeyDown = (event: React.KeyboardEvent) => { + // Implement keyboard navigation logic + }; + return <RovingFocusGroupContext.Provider value={{ focusedItemId, setFocusedItemId }}> - <Primitive.div {...props}> + <Primitive.div + role="group" + onKeyDown={handleKeyDown} + data-orientation={orientation} + dir={dir} + {...props} + > {children} </Primitive.div> </RovingFocusGroupContext.Provider>; };
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
18-21: Remove or replace leftover debugging statements.The repeated console logging seems to be for debugging and could clutter production logs.
- console.log('focusedItemId', focusedItemId); - console.log('groupRef', groupRef); - console.log('groupRef.current', groupRef.current); + // Remove or replace with a proper debug utility or logger if needed.
74-77: Replace placeholder tab logic.The
Tabkey handling is just logging to the console. Either remove it or implement real tab behavior if required by your design.case 'Tab': - console.log('Tab'); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
5-5: Fix typo in import path (confirmed from past feedback).This import path is missing the 's' in
Focus. The correct path should be'../context/RovingFocusGroupContext'.
83-86: Confirm necessity ofevent.preventDefault()on focus.Preventing the default focus behavior could interfere with standard browser accessibility and tabbing behavior. Verify that this is intended.
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (2)
4-4: Fix typo in import path (confirmed from past feedback).As noted previously, ensure the import uses
RovingFocusGroupContext(with an 's') for correctness.
12-37: Looks good overall!The component properly initializes focus items and sets the first item as focused by default. The
loopproperty is passed down to context, which is handled inRovingFocusItem. No immediate issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
11-30: Remove debug statements or wrap them behind a development flag.Console logs (
console.log) within the effect can create noise in production. Consider removing these logs or wrapping them in a conditional check to avoid cluttering the console.
32-97: Refactor duplicated navigation logic.The keydown navigation code for ArrowUp/ArrowLeft/ArrowDown/ArrowRight is largely repetitive. Consider extracting common code (e.g., finding the previous/next index) into helper functions to reduce duplication and simplify maintenance.
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (1)
9-37: Combine more usage examples or document different scenarios.The default story effectively demonstrates two groups, but adding additional scenarios (e.g., dynamic item counts, disabled items) or further documentation can help highlight the roving focus functionality more thoroughly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
5-5: Fix the import path to include the missing 's' in "Focus".It appears that there's a spelling mismatch in the import statement:
'../context/RovingFocuGroupContext'. Based on previous discussions, this should likely be'../context/RovingFocusGroupContext'.
99-102: Verify usage ofpreventDefault()on focus event.Calling
event.preventDefault()in a focus event might interfere with default focus behavior. Ensure this aligns with your design intentions, particularly if you want the element to naturally receive focus.
104-107: Add ARIA attributes to enhance accessibility.Consider configuring accessibility attributes (e.g.,
role="menuitem",aria-selected) on thePrimitive.buttonto ensure proper screen reader and keyboard navigation support.src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (2)
40-46: Horizontal story looks good.
48-54: Vertical story implementation is clear and consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/ui/Tabs/fragments/TabTrigger.tsx (1)
10-18:⚠️ Potential issueFix type error in TabTriggerProps interface.
There's a type error in the TabTriggerProps interface -
setActiveTabis defined withReact.Dispatch<Tab>butTabis not defined. It should likely beReact.Dispatch<React.SetStateAction<string>>or similar based on how it's used.export type TabTriggerProps = { tab: TabProps; - setActiveTab: React.Dispatch<Tab>; + setActiveTab: React.Dispatch<React.SetStateAction<string>>; activeTab: TabProps; className?: string; customRootClass?: string; index: number; props?: Record<string, any>[] }
♻️ Duplicate comments (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
5-5:⚠️ Potential issueFix the typo in the import path.
The file name in the import statement appears to be missing an “s.” This has been mentioned in a previous review.Apply this diff to correct the import path:
-import { RovingFocusGroupContext } from '../context/RovingFocuGroupContext'; +import { RovingFocusGroupContext } from '../context/RovingFocusGroupContext';
🧹 Nitpick comments (4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
11-27: Include all referenced variables in the dependency array.
The effect referencesgroupRef,id, andaddFocusItembut excludes them from the dependency array. This can lead to stale or inconsistent behavior.useEffect(() => { if (!focusItems.includes(id)) { addFocusItem(id); } if (focusedItemId === id) { if (groupRef.current) { const sanitizedId = CSS.escape(id); const item = groupRef.current.querySelector(`#${sanitizedId}`); if (item) { item.focus(); } } } -}, [focusItems, focusedItemId]); +}, [focusItems, focusedItemId, groupRef, id, addFocusItem]);
29-94: Remove leftover debug statement for the Tab key.
Theconsole.log('Tab')might be a placeholder. Consider removing or replacing it with any required Tab-handling logic to keep production code clean.case 'Tab': - console.log('Tab'); break;
101-103: Add appropriate ARIA attributes for consistent accessibility.
Since the parent usesrole="tablist", consider marking individual items withrole="tab"and indicate selection state.return <Primitive.button asChild onFocus={handleFocus} tabIndex={focusedItemId === id ? 0 : -1} ref={ref} id={id} onKeyDown={handleKeyDown} - {...props}> + role="tab" + aria-selected={focusedItemId === id} + {...props}> {children} </Primitive.button>;src/components/ui/Tabs/fragments/TabTrigger.tsx (1)
27-36: Consider refactoring handleFocus function.The handleFocus function contains redundant code and could be simplified:
- The check
if (ref.current)is unnecessary as it's immediately followed byref.current?.focus()which already has the optional chaining operator- The comments are verbose and could be more concise
const handleFocus = (tab: TabProps) => { - if (ref.current) { - ref.current?.focus(); - } + ref.current?.focus(); setActiveTab(tab.value); - - // This is a good way to manage flow, when a focus event is triggered, we can set the active tab to the tab that is being focused on - // This way, we dont need to keep track of the active tab in the parent component - // This should be the defacto pattern we should follow for all components };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/ui/Tabs/fragments/TabList.tsx(2 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(2 hunks)src/components/ui/Tabs/fragments/TabTrigger.tsx(3 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (9)
src/components/ui/Tabs/fragments/TabList.tsx (2)
7-8: Import statement looks good.
UsingRovingFocusGrouphere is consistent with the rest of the codebase. No issues detected.
21-23: Tab list container migration to RovingFocusGroup.Group
Replacing the<div>with<RovingFocusGroup.Group>is a good step towards better focus management. Therole="tablist"andaria-orientation="horizontal"are correct for tab navigation.src/components/ui/Tabs/fragments/TabRoot.tsx (2)
8-8: New import for RovingFocusGroup is valid.
No concerns with the import path or usage here.
29-32: Adoption of RovingFocusGroup.Root
Wrapping the content with<RovingFocusGroup.Root>ensures cohesive focus handling for the tab structure. The usage here is appropriate, and the custom data attribute (data-accent-color) appears benign.src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
96-99: Revisit preventing default focus behavior.
Callingevent.preventDefault()on a focus event can cause unexpected accessibility issues. Unless you have a specific reason, consider removing it to allow normal focus handling.const handleFocus = (event: React.FocusEvent<HTMLButtonElement>) => { - event.preventDefault(); setFocusedItemId(id); };
106-108: Display name and export.
No issues with thedisplayNameor default export.src/components/ui/Tabs/fragments/TabTrigger.tsx (3)
22-22: Removed explicit keyboard navigation pattern.The change simplifies the context usage by removing
previousTabandnextTabfunctions, offloading this responsibility to the RovingFocusGroup component. This is a good architectural decision that centralizes keyboard navigation logic.
39-48:❓ Verification inconclusive
Verify accessibility with RovingFocusGroup implementation.
The component now uses a composition pattern with RovingFocusGroup.Item and the
asChildprop. This change removes the explicit keyboard event handling that was previously managing tab navigation, relying instead on the RovingFocusGroup utility.Make sure the RovingFocusGroup implementation properly handles:
- Arrow key navigation between tabs
- Home/End key navigation
- Screen reader announcements
- Focus containment within the tab list
🏁 Script executed:
#!/bin/bash # Check RovingFocusGroup.Item implementation for keyboard navigation echo "Checking RovingFocusGroup.Item implementation:" rg -A 20 "RovingFocusGroup.Item = " src/ # Check if there are any tests for RovingFocusGroup echo -e "\nChecking for tests:" fd "RovingFocusGroup.*test|test.*RovingFocusGroup" src/Length of output: 258
Accessibility Verification: RovingFocusGroup Integration
The updated component now relies on the RovingFocusGroup’s built-in focus management via the composition pattern (with
asChild), removing explicit keyboard event handling. Please ensure that the RovingFocusGroup implementation exhibits correct behavior regarding:
- Arrow key navigation between tabs.
- Home/End key navigation to jump to the first/last tab.
- Screen reader announcements for state changes.
- Focus containment within the tab list.
Note: Our search did not reveal any internal tests validating these accessibility features. It would be beneficial to manually verify these behaviors and consider adding tests if they are not already covered by the RovingFocusGroup utility.
8-9:✅ Verification successful
Verify integration with RovingFocusGroup utility.
The new import of RovingFocusGroup represents a significant shift in how focus management is handled in the tabs component. This is a good architectural change that abstracts focus management logic.
🏁 Script executed:
#!/bin/bash # Check how RovingFocusGroup is implemented and used in other components echo "Checking RovingFocusGroup implementation:" cat src/core/utils/RovingFocusGroup.tsx || cat src/core/utils/RovingFocusGroup/index.ts || fd "RovingFocusGroup" src/core/utils --exec cat {} echo -e "\nChecking usage across the codebase:" rg -t tsx "import .* RovingFocusGroup" src/ --statsLength of output: 3981
I'll run an additional check to verify how the
RovingFocusGroupis integrated within the Tabs component.
🏁 Script executed:
#!/bin/bash # Inspect the content of TabTrigger to see RovingFocusGroup usage echo "Inspecting RovingFocusGroup integration in TabTrigger.tsx:" rg "RovingFocusGroup" src/components/ui/Tabs/fragments/TabTrigger.tsx -nLength of output: 333
Integration with RovingFocusGroup verified successfully.
- The import in
src/components/ui/Tabs/fragments/TabTrigger.tsx(line 8) is correctly referencing the utility, and its usage (lines 39 and 48) aligns with the intended focus management abstraction.- The implementation of
RovingFocusGroupwithinsrc/core/utils/RovingFocusGroupappears consistent with its usage in other parts of the codebase.Overall, the architectural change to abstract focus management with
RovingFocusGrouphas been integrated properly.
kotAPI
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (4)
7-7: Add extended props to support disabled/focusable states.To align with best practices for roving focus items, consider adding props like
disabledandfocusable, as recommended in a past review.
34-99: Consider proactive handling of arrow key default behavior.While you manage focus movement on arrow keys, you might also consider calling
event.preventDefault()for arrow presses to prevent unintended native scrolling, especially for horizontally oriented groups.
93-95: Remove leftover debug statement.Debug statements like
console.log('Tab')can clutter logs in production.-case 'Tab': - console.log('Tab'); - break; +case 'Tab': + // handle tab logic if needed + break;
106-108: Add accessibility attributes for better screen reader support.AROA/ARIA attributes (e.g.,
role="menuitem"oraria-selected) should be added to help assistive technologies identify the focused element.return <Primitive.button asChild onFocus={handleFocus} tabIndex={focusedItemId === id ? 0 : -1} ref={ref} id={id} onKeyDown={handleKeyDown} + role="menuitem" + aria-selected={focusedItemId === id} {...props}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
5-5: Fix the typo in the import path.The import path for
RovingFocusGroupContextis missing an "s," which breaks the linkage.-import { RovingFocusGroupContext } from '../context/RovingFocuGroupContext'; +import { RovingFocusGroupContext } from '../context/RovingFocusGroupContext';
101-104: Revisit thepreventDefault()call on focus.Preventing the default focus behavior can interfere with standard browser focus. Verify whether you need this or if you can rely on the default event handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
13-22: 🛠️ Refactor suggestionAdd missing dependencies to the effect hook
addFocusItemandidare used inside the effect but aren’t listed in the dependencies array. This may lead to stale references or unexpected behavior.useEffect(() => { if (!focusItems.includes(id)) { addFocusItem(id); } -}, [focusItems, focusedItemId]); +}, [focusItems, focusedItemId, addFocusItem, id]);
🧹 Nitpick comments (5)
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (1)
7-17: Consider adding an ARIA role or label to improve accessibilityExplicitly assigning a role (e.g.,
role="group") or relevant ARIA attributes to the container might help assistive technologies identify how to treat this interactive wrapper.src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
95-97: Remove or replace console.log for productionLeaving a console log for the Tab key can clutter logs in production environments. Consider removing or using a more robust logging mechanism.
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (1)
16-18: Prevent duplicates infocusItemsThe current approach appends all IDs to the list without checking for duplicates, which can cause repeated entries.
const addFocusItem = (id: string) => { - setFocusItems((prev) => [...prev, id]); + setFocusItems((prev) => { + if (!prev.includes(id)) { + return [...prev, id]; + } + return prev; + }); };src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (2)
1-52: Add usage instructions to demonstrate keyboard navigationThe story doesn't explicitly indicate to users how to interact with the component. Consider adding instructions about keyboard navigation, which is the core functionality of a RovingFocusGroup.
import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; import React from 'react'; import RovingFocusGroup from '../index'; import Button from '~/components/ui/Button/Button'; // More on how to set up stories at: https://storybook.js.org/docs/react/writing-stories/introduction#default-export export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + parameters: { + docs: { + description: { + component: 'RovingFocusGroup manages keyboard navigation between focusable elements. Press Tab to enter a group, then use arrow keys to navigate between items within the group.' + } + } + }, render: (args) => <> <SandboxEditor className="space-y-2"> + <div className="mb-4 p-2 bg-gray-100 rounded"> + <h3 className="font-bold">Keyboard Navigation:</h3> + <ul className="list-disc pl-5"> + <li>Press <kbd>Tab</kbd> to focus the first group</li> + <li>Use <kbd>←</kbd> <kbd>→</kbd> or <kbd>↑</kbd> <kbd>↓</kbd> to navigate between items (depending on direction)</li> + <li>Press <kbd>Tab</kbd> again to move to the next group</li> + </ul> + </div> <RovingFocusGroup.Root className="flex items-center gap-2" {...args}>
12-36: Consider adding customization controls for interactive testingThe story would be more useful if it allowed interactive testing of different properties. Consider exposing more controls via args.
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + argTypes: { + direction: { + control: 'radio', + options: ['horizontal', 'vertical'], + description: 'Navigation direction' + }, + loop: { + control: 'boolean', + description: 'Whether focus should loop from last to first item' + } + }, render: (args) => <>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/ui/Tabs/fragments/TabContent.tsx(1 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(2 hunks)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/Tabs/fragments/TabRoot.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (11)
src/components/ui/Tabs/fragments/TabContent.tsx (2)
19-19: Clean removal of unused setActiveTab dependency.The removal of
setActiveTabfrom the context destructuring is a good cleanup that aligns with the refactoring to use RovingFocusGroup for focus management. The TabContent component only needs to read the current state (tabsandactiveTab) without modifying it.
23-23: Potential type mismatch in activeTab comparison.There appears to be a type inconsistency between how
activeTabis typed and how it's used. TheTabContentPropstype definesactiveTabas aTabPropsobject (line 11), but here it's compared againsttab.value, suggestingactiveTabmight actually be a string/value identifier rather than a full tab object.Consider adjusting either:
- The type definition to reflect that
activeTabis a value identifier:- activeTab: TabProps + activeTab: string | number
- Or the comparison logic if
activeTabis indeed a full tab object:- if (tab.value === activeTab) { + if (tab.value === activeTab.value) {src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (1)
7-7: Looks good overallNo major issues spotted in the component logic or context usage.
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
5-5: Check for possible typo in import pathThe import path is referencing
'../context/RovingFocuGroupContext'. Verify if the file name is spelled “Focu” or “Focus.”Do you want me to confirm the file name usage throughout the codebase?
103-106: Reevaluate preventing default on focusCalling
event.preventDefault()within the onFocus handler can block native focus behaviors. Ensure this is desired or remove it.
108-110: Add ARIA attributes to the button element for improved accessibilitysrc/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (1)
4-4: Check for possible typo in the import pathIf the actual file is named
RovingFocusGroupContext.ts, consider updating the path for consistency.src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (4)
8-37: Add documentation parameters to the storyThe story setup is functional but lacks documentation parameters that would explain the purpose and usage of the RovingFocusGroup component to other developers. Consider adding descriptive parameters to enhance documentation.
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + parameters: { + docs: { + description: { + component: 'RovingFocusGroup manages keyboard navigation between focusable elements, implementing the roving tabindex pattern for accessibility.' + } + } + }, render: (args) => <>
14-34: Great implementation of example groups in RovingFocusGroupThe story effectively demonstrates the component with multiple groups and different types of focusable elements (buttons and links). The visual distinction between groups using colored borders is helpful.
39-45: Add documentation to story variantsWhile you've implemented the horizontal and vertical variants, they lack descriptive parameters explaining what each variant demonstrates and how to use them.
export const Horizontal = { + parameters: { + docs: { + description: { + story: 'Horizontal navigation using left/right arrow keys. Setting loop=true allows focus to wrap from last to first item.' + } + } + }, args: { direction: 'horizontal', loop: true } };
47-52: Add documentation to the Vertical variantSimilar to the Horizontal variant, the Vertical variant would benefit from documentation explaining its behavior.
export const Vertical = { + parameters: { + docs: { + description: { + story: 'Vertical navigation using up/down arrow keys. Setting loop=true allows focus to wrap from last to first item.' + } + } + }, args: { direction: 'vertical', loop: true } };
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/tools/SandboxEditor/SandboxEditor.tsx (5)
25-32: Consider simplifying ColorSelect componentThe ColorSelect component has been changed to return a
<span>but it's later wrapped in a<button>when used within RovingFocusGroup.Item. This creates unnecessary nesting.Consider having ColorSelect return null or just a color indicator and apply styling at the RovingFocusGroup.Item level:
const ColorSelect = ({ color, colorName, changeAccentColor }: ColorSelectProps) => { const dimensions = 32; - return <span - aria-label={`Change accent color to ${colorName}`} - className='cursor-pointer rounded-full inline-flex' - style={{ width: dimensions, height: dimensions, backgroundColor: color.light['900'] }} - ></span>; + return ( + <div + style={{ width: dimensions, height: dimensions, backgroundColor: color.light['900'] }} + ></div> + ); };
71-90: Good implementation of RovingFocusGroup for color selectionThe implementation of RovingFocusGroup enhances keyboard navigation and focus management for color selection. This improves accessibility by allowing users to navigate between color options using keyboard.
This is a good example of how RovingFocusGroup improves user interaction. Consider applying this pattern to other groups of interactive elements throughout the application that would benefit from improved keyboard navigation.
78-80: Enhance color selection feedbackThe current implementation updates the color selection on both focus and click. For better user experience, consider adding some visual feedback to indicate which color is currently selected.
<RovingFocusGroup.Item asChild key={idx} - className='cursor-pointer rounded-full inline-block w-8 h-8 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500' + className={`cursor-pointer rounded-full inline-block w-8 h-8 focus:outline-none focus:ring-2 focus:ring-offset-2 ${colorName === color ? 'ring-2 ring-offset-2 ring-blue-500' : 'focus:ring-blue-500'}`} onFocus={() => setColorName(colorName)} onClick={() => setColorName(colorName)} >
23-23: Unused parameter in ColorSelect propsThe
changeAccentColorparameter in ColorSelect props is defined but never used. It seems redundant since the color change is now handled by the RovingFocusGroup.Item's onClick and onFocus handlers.-type ColorSelectProps = {color:typeof colors[keyof typeof colors], colorName: string, changeAccentColor: (colorName: string)=>void} +type ColorSelectProps = {color:typeof colors[keyof typeof colors], colorName: string}And update usage:
-<ColorSelect colorName={color} color={colors[colorName]}/> +<ColorSelect colorName={color} color={colors[colorName]}/>
43-45: Remove empty useEffectThere's an empty useEffect hook that doesn't perform any actions. This should be removed if it's not needed.
-useEffect(() => { - -}, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/tools/SandboxEditor/SandboxEditor.tsx(4 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/components/tools/SandboxEditor/SandboxEditor.tsx
[warning] 1-1:
'forwardRef' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/components/tools/SandboxEditor/SandboxEditor.tsx (2)
7-8: Good addition of RovingFocusGroup importThe import of
RovingFocusGroupaligns with the PR objective to refactor components to use the new roving focus management utility.
59-59: Button variant change to 'solid'The button variant has been changed from "outline" to "solid" for the dark mode toggle, which provides better visual prominence.
| @@ -1,9 +1,11 @@ | |||
| import React, { PropsWithChildren, useEffect, useState } from 'react'; | |||
| import React, { forwardRef, PropsWithChildren, useEffect, useState } from 'react'; | |||
There was a problem hiding this comment.
Remove unused import 'forwardRef'
The forwardRef import is defined but never used in this component.
-import React, { forwardRef, PropsWithChildren, useEffect, useState } from 'react';
+import React, { PropsWithChildren, useEffect, useState } from 'react';📝 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.
| import React, { forwardRef, PropsWithChildren, useEffect, useState } from 'react'; | |
| import React, { PropsWithChildren, useEffect, useState } from 'react'; |
🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'forwardRef' is defined but never used
| <button> | ||
| <ColorSelect colorName={color} color={colors[colorName]}/> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve aria-label for accessibility
The aria-label from ColorSelect is lost because it's wrapped in a button that doesn't forward the attributes.
<button
+ aria-label={`Change accent color to ${color}`}
>
<ColorSelect colorName={color} color={colors[colorName]}/>
</button>📝 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.
| <button> | |
| <ColorSelect colorName={color} color={colors[colorName]}/> | |
| </button> | |
| <button aria-label={`Change accent color to ${color}`}> | |
| <ColorSelect colorName={color} color={colors[colorName]}/> | |
| </button> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (4)
8-10: Consider extending the props for configurable interactions.Defining props such as
disabledorfocusablecan make this component more flexible, align with usage patterns across the codebase, and allow conditionally disabling focus logic.
28-39: Use optional chaining for clarity.Instead of checking
if (groupRef && groupRef.current), consider using optional chaining (groupRef?.current) to reduce nesting and improve readability.- if (groupRef && groupRef.current) { + if (groupRef?.current) { setFocusedItemId(id); // ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
40-105: Refine handling of the Tab key.Currently, pressing Tab logs a message without altering focus behavior. If you intend to allow default tabbing, remove the console statement; otherwise, implement the desired focus changes or constraints.
-case 'Tab': - console.log('Tab'); - break; +case 'Tab': + // Let the default browser tab behavior proceed + break;
112-115: Add accessibility attributes for better screen reader support.Providing ARIA attributes like
role,aria-selected, and others can improve the user experience for assistive technologies. Consider specifying the exact role and selection state to reflect the item’s focus or selection status.<Primitive.button asChild onFocus={handleFocus} tabIndex={focusedItemId === id ? 0 : -1} ref={ref} id={id} onKeyDown={handleKeyDown} + role="menuitem" + aria-selected={focusedItemId === id} {...props} > {children} </Primitive.button>src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx (1)
14-21: Optional improvement: Throw an error in default context values to prevent silent misconfigurations.Currently, the default context values (like
setFocusedItemId: () => {}) silently no-op if the component isn’t wrapped by a provider. Consider throwing an error in these methods to alert maintainers that they must wrap usage in the provider.export const RovingFocusGroupContext = createContext<RovingFocusGroupContextTypes>({ focusedItemId: null, - setFocusedItemId: () => {}, + setFocusedItemId: () => { + throw new Error('RovingFocusGroupContext missing provider'); + }, // ... });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx
- src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx
- src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx
🧰 Additional context used
🧬 Code Definitions (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx (2) (2)
RovingFocusGroupContextTypes(4-11)RovingFocusGroupContext(14-21)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (2) (2)
RovingFocusRootContextTypes(4-7)RovingFocusRootContext(10-13)
🪛 Biome (1.9.4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
107-110: Verify the usage ofevent.preventDefault()during focus.Calling
event.preventDefault()insideonFocusis unusual and may disrupt built-in focus handling. Double-check whether this is intended or if it should be removed for expected tab navigation behavior.
117-119: LGTM: The displayName and default export are clear.Defining a
displayNameis helpful for debugging. This finalizes the component setup nicely.
1-6:✅ Verification successful
Confirm React version support for
useId.Since
useIdis only available in React 18 or higher, please confirm that your project is running at least React 18 to avoid runtime errors.
🌐 Web query:
Does React 17 provide the `useId` hook or is it only available from React 18 onwards?💡 Result:
The
useIdhook is only available in React 18 and later versions. React 17 does not include this hook. Here's the breakdown:
Introduced in React 18: The
useIdhook was added in React 18 to generate stable, unique IDs across client-server renders[1][3][11]. It addresses issues like duplicate IDs in reusable components and ensures hydration consistency[1][4].React 17 Limitations: While React 17 included foundational updates like event system changes and JSX transform improvements[2][8][12], it lacked
useId. Developers using React 17 had to rely on third-party solutions like@reach/auto-idfor similar functionality[4].Key Differences:
React 17 React 18+ No native unique ID generator useIdbuilt-in[1][11]Required manual ID management Automatic ID generation tied to component hierarchy[4][11] Risk of ID collisions in SSR Hydration-safe IDs with useId[1][4]For projects using React 17, upgrading to React 18 is required to use
useIdnatively[3][19], or alternatives likereact-use-id-hook(via npm) can be used as a polyfill[7].Citations:
- 1: https://www.telerik.com/blogs/react-useid-hook
- 2: https://webapplog.com/whats-new-in-react-17/
- 3: tailwindlabs/headlessui#1009
- 4: https://reacttraining.com/blog/use-useid-instead-of-hand-making-ids
- 5: https://blog.radialcode.com/category/web/comparison-between-react-17-and-react-18
- 6: https://en.wikipedia.org/wiki/React_(software)
- 7: https://www.npmjs.com/package/react-use-id-hook
- 8: https://legacy.reactjs.org/blog/2020/08/10/react-v17-rc.html
- 9: https://github.com/react-component/util/blob/master/src/hooks/useId.ts
- 10: https://legacy.reactjs.org/blog/2020/10/20/react-v17.html
- 11: https://react.dev/reference/react/useId
- 12: https://nextlinklabs.com/resources/insights/react-16-vs-17
- 13: https://www.youtube.com/watch?v=n8hzemj6iYg
- 14: https://javascript.plainenglish.io/the-biggest-change-in-react-17-that-no-one-is-talking-about-b62905beb2d2
- 15: https://react.dev/reference/react/hooks
- 16: https://www.opcito.com/blogs/react-17-vs-18-vs-19-whats-the-difference
- 17: https://legacy.reactjs.org/docs/hooks-reference.html
- 18: https://react.dev/versions
- 19: AnyRoad/react-json-view-lite#20
- 20: https://www.shecodes.io/athena/1194-comparing-react-17-and-react-18-features
Ensure Your Project Uses React 18 or Later for
useIdSupport
Based on our verification, theuseIdhook is only available in React 18 or newer versions and is not present in React 17. Please confirm that your project's dependency is set to React 18+ (check yourpackage.json) to avoid any runtime issues. If you are currently on React 17, consider upgrading or implementing a polyfill solution.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (4)
17-26:⚠️ Potential issueFix the dependency array in useEffect.
The useEffect hook is missing dependencies
idandaddFocusItemwhich are used inside the effect. This can lead to stale closures and missed updates.useEffect(() => { // we check if the item is in the focusItems array, if not we add it if (!focusItems.includes(id)) { addFocusItem(id); } // // -}, [focusItems, focusedItemId]); +}, [focusItems, focusedItemId, id, addFocusItem]);
119-122: 🛠️ Refactor suggestionAdd disabled state handling in focus logic.
The component should respect a disabled state and not allow focus when disabled.
+interface RovingFocusItemProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { + children: React.ReactNode; + disabled?: boolean; +} // Then in the component implementation: const handleFocus = (event: React.FocusEvent<HTMLButtonElement>) => { event.preventDefault(); - setFocusedItemId(id); + if (!props.disabled) { + setFocusedItemId(id); + } };
124-126: 🛠️ Refactor suggestionAdd ARIA attributes for better accessibility.
The button element is missing important ARIA attributes that help screen readers understand the role and state of the element.
-return <Primitive.button asChild onFocus={handleFocus} tabIndex={focusedItemId === id ? 0 : -1} ref={ref} id={id} onKeyDown={handleKeyDown} {...props}> +return <Primitive.button + asChild + onFocus={handleFocus} + tabIndex={focusedItemId === id ? 0 : -1} + ref={ref} + id={id} + onKeyDown={handleKeyDown} + role="menuitem" + aria-selected={focusedItemId === id} + {...props} +> {children} </Primitive.button>;
8-10: 🛠️ Refactor suggestionEnhance component props interface with additional configuration.
The current props interface is limited and doesn't include important properties for a proper roving focus implementation, like
disabledstate.-type RovingFocusItemProps = { - children: React.ReactNode; -} & React.ButtonHTMLAttributes<HTMLButtonElement>; +interface RovingFocusItemProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { + children: React.ReactNode; + disabled?: boolean; + focusable?: boolean; +}
🧹 Nitpick comments (8)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
23-25: Remove empty comment lines.These empty comment lines don't provide any value and should be removed.
if (!focusItems.includes(id)) { addFocusItem(id); } - // - - // }, [focusItems, focusedItemId]);
29-30: Use optional chaining for cleaner code.The current null check can be simplified using optional chaining for better readability.
- const focusItemWithId = (id: string) => { - if (groupRef && groupRef.current) { - setFocusedItemId(id); + const focusItemWithId = (id: string) => { + setFocusedItemId(id); + if (groupRef?.current) {🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
110-113: Remove console.log statement.There's a console.log statement that appears to be leftover debugging code and should be removed. Consider implementing proper Tab key handling instead.
case 'Tab': - // Logic to handle tab key if needed - console.log('Tab'); break;src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (5)
9-12: Type property mismatch:directionvsorientationThe
StoryArgstype definesdirectionbut the previous review comments mentionedorientation. Ensure consistency in the API naming throughout the component.type StoryArgs = { - direction?: 'horizontal' | 'vertical'; + orientation?: 'horizontal' | 'vertical'; loop?: boolean; }; // In Horizontal component args: { - direction: 'horizontal', + orientation: 'horizontal', loop: true }Also applies to: 48-49
16-17: Add documentation parameters to improve component understandingThe story lacks documentation describing what the RovingFocusGroup component does. Consider adding parameters with a description explaining the component's purpose and accessibility benefits.
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + parameters: { + docs: { + description: { + component: 'RovingFocusGroup implements the roving tabindex pattern for keyboard navigation between elements. It prevents default browser scrolling when using arrow keys and manages focus order.' + } + } + }, render: (args: StoryArgs) => <>
29-29: Add aria-label to link for accessibilityLinks should have descriptive labels for screen readers. Since this is a demonstration of an accessibility pattern, it's important to model best practices.
-<a href="#" className="border border-green-500">Link 1 Group 1</a> +<a href="#" className="border border-green-500" aria-label="Link 1 Group 1">Link 1 Group 1</a>
18-42: Consider consistency in story render implementationThe default export uses a function component syntax while KeyboardNavigation uses an object with a render property. For consistency, consider using the same pattern throughout.
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, - render: (args: StoryArgs) => <> + render: (args: StoryArgs) => ( <SandboxEditor className="space-y-2"> {/* ... */} </SandboxEditor> - </> + ) };Also applies to: 62-153
1-7: Consider adding missing import documentationSince this is a new component that others will reference, add a brief comment explaining what
RovingFocusGroupis and how it relates to this story file.import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; import React from 'react'; +/** + * RovingFocusGroup is a utility that implements the roving tabindex pattern + * for keyboard navigation. It allows users to navigate between elements + * using arrow keys without triggering page scrolling. + */ import RovingFocusGroup from '../index'; import Button from '~/components/ui/Button/Button';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx (2) (2)
RovingFocusGroupContextTypes(4-11)RovingFocusGroupContext(14-21)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (2) (2)
RovingFocusRootContextTypes(4-7)RovingFocusRootContext(10-13)
🪛 Biome (1.9.4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (2)
46-58: Expand documentation for the orientation variantsThe current story variants don't provide documentation to explain their purpose. Consider adding parameter descriptions for both stories.
export const Horizontal = { + parameters: { + docs: { + description: { + story: 'Demonstrates horizontal navigation with left/right arrow keys. The loop parameter ensures focus wraps from last to first item and vice versa.' + } + } + }, args: { direction: 'horizontal', loop: true } }; export const Vertical = { + parameters: { + docs: { + description: { + story: 'Demonstrates vertical navigation with up/down arrow keys. The loop parameter ensures focus wraps from last to first item and vice versa.' + } + } + }, args: { direction: 'vertical', loop: true } };
60-154: Good implementation of comprehensive keyboard navigation exampleThe KeyboardNavigation story provides an excellent demonstration of the component's functionality with clear explanations and multiple scenarios. The descriptions and visual indicators make it easy to understand how the component prevents default scrolling behavior while enabling keyboard navigation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (1)
67-77: Consider using optional chaining for cleaner codeYou can simplify the null check by using optional chaining.
- const focusItemWithId = (id: string) => { - if (groupRef && groupRef.current) { - setFocusedItemId(id); - // Sanitize the id to ensure it's a valid CSS selector - const sanitizedId = CSS.escape(id); - const item = groupRef.current.querySelector(`#${sanitizedId}`); - if (item) { - (item as HTMLElement).focus(); - } - } - }; + const focusItemWithId = (id: string) => { + if (groupRef?.current) { + setFocusedItemId(id); + // Sanitize the id to ensure it's a valid CSS selector + const sanitizedId = CSS.escape(id); + const item = groupRef.current.querySelector(`#${sanitizedId}`); + if (item) { + (item as HTMLElement).focus(); + } + } + };🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (5)
9-12: Type property name doesn't match component props.The
StoryArgstype usesdirectionbut the component likely usesorientationbased on the Storybook variants. This inconsistency could cause confusion.type StoryArgs = { - direction?: 'horizontal' | 'vertical'; + orientation?: 'horizontal' | 'vertical'; loop?: boolean; };
29-30: Replace empty href with more accessible alternative.Using
href="#"can cause page jumps and isn't ideal for accessibility. Consider using a button styled as a link or a more meaningful href.- <a href="#" className="border border-green-500">Link 1 Group 1</a> + <a href="javascript:void(0)" className="border border-green-500">Link 1 Group 1</a>Alternatively, for a more semantic approach:
- <a href="#" className="border border-green-500">Link 1 Group 1</a> + <button className="border border-green-500 text-blue-600 underline">Link 1 Group 1</button>
147-151: Consider a more accessible approach for demonstrating scroll prevention.While adding empty space to make the page scrollable works for demonstration, it's not very screen-reader friendly. Consider using actual content or a more descriptive approach.
- {/* Add empty space to make the page scrollable */} - <div className="h-[600px] bg-gray-100" /> + {/* Add content to make the page scrollable */} + <div className="space-y-4 bg-gray-100 p-4"> + <h3 className="text-md font-medium">Scrollable Content Area</h3> + <p>This area contains content to make the page scrollable for demonstration purposes.</p> + <div className="h-[500px] flex items-end"> + <p className="text-sm text-gray-600">Scroll to bottom content...</p> + </div> + </div>
177-178: Use a consistent pattern for disabled items.You're using both inline styles and the Button component's built-in disabled state. Consider adopting a consistent approach across all examples.
If the Button component handles disabled styling internally:
- <Button disabled className="opacity-50 cursor-not-allowed">Disabled Item</Button> + <Button disabled>Disabled Item</Button>If not, ensure all disabled buttons use the same pattern:
- <Button disabled>Admin (Disabled)</Button> + <Button disabled className="opacity-50 cursor-not-allowed">Admin (Disabled)</Button>Also applies to: 183-184, 205-206, 211-212, 230-231, 236-237, 292-293
19-41: Consider adding a basic description to the default story.The default render function shows the component structure well, but lacks a description explaining what's being demonstrated (multiple groups within a single focus root).
export default { title: 'WIP/RovingFocusGroup', component: RovingFocusGroup, + parameters: { + docs: { + description: { + component: 'RovingFocusGroup manages keyboard navigation within and between groups of focusable elements, implementing the roving tabindex pattern.' + } + } + }, render: (args: StoryArgs) => <>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(1 hunks)src/core/utils/RovingFocusGroup/index.tsx(1 hunks)src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx
- src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx
🧰 Additional context used
🧬 Code Definitions (3)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (2)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx (1) (1)
RovingFocusGroupContext(26-33)src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (1) (1)
RovingFocusRootContext(17-20)
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (1)
src/core/utils/RovingFocusGroup/context/RovingFocusRootContext.tsx (1) (1)
RovingFocusRootContext(17-20)
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (1)
src/core/utils/RovingFocusGroup/context/RovingFocusGroupContext.tsx (1) (1)
RovingFocusGroupContext(26-33)
🪛 Biome (1.9.4)
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx
[error] 68-68: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (11)
src/core/utils/RovingFocusGroup/index.tsx (2)
5-44: Well-documented component with comprehensive JSDocThe JSDoc documentation for this component is excellent. It clearly explains the purpose, key features, and accessibility benefits of the RovingFocusGroup utility. The code example is particularly helpful for understanding how to use the component.
45-49: Clean and consistent component composition patternGood job using the compound component pattern here. This approach of exposing Root, Group, and Item as properties of a single exported object provides a clean and intuitive API for consumers.
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (2)
15-21: Well-defined props interface with thorough documentationThe props interface is clearly defined with good JSDoc comments. The optional props have sensible defaults, and the component extends HTMLDivElement attributes to ensure full compatibility with standard HTML attributes.
50-60: Proper context implementation and ARIA attributesThe component correctly uses the RovingFocusRootContext provider to pass configuration values down to child components. The ARIA attributes are properly applied for accessibility, matching the WAI-ARIA best practices mentioned in the documentation.
src/core/utils/RovingFocusGroup/fragments/RovingFocusItem.tsx (3)
85-117: Robust implementation of enabled item finding logicThe
findEnabledItemIndexfunction is well-implemented with thorough handling of edge cases:
- Properly respects the loop configuration
- Avoids infinite loops with a boundary check
- Correctly identifies disabled items
- Uses CSS.escape for secure DOM querying
This is a solid implementation that ensures reliable keyboard navigation.
175-226: Comprehensive keyboard navigation implementationThe keyboard event handling is excellent, with support for:
- All directional arrow keys
- Home and End keys for jumping to first/last items
- Direction-aware navigation (horizontal/vertical)
- Preventing default scrolling behavior
- Properly skipping disabled items
This implementation follows accessibility best practices and provides a great user experience.
238-254: Complete ARIA attributes for accessibilityThe component correctly implements all necessary ARIA attributes:
- role="option"
- aria-selected
- aria-disabled
- aria-label and aria-labelledby support
- Proper tabIndex management
This ensures the component is fully accessible to screen readers and keyboard users.
src/core/utils/RovingFocusGroup/fragments/RovingFocusGroup.tsx (2)
38-55: Well-implemented focus state managementThe state management for focus items is well-implemented:
- The
focusItemsarray tracks all available items- The
addFocusItemfunction correctly adds new items- The effect to set initial focus works properly
This ensures that focus management is reliable and predictable.
66-77: Proper context provider and ARIA attributesThe component correctly provides the focus management context to child components and applies appropriate ARIA attributes for accessibility. The use of a ref to access the DOM node is also a good practice for complex interactions.
src/core/utils/RovingFocusGroup/stories/RovingFocusGroup.stories.tsx (2)
16-17: Add documentation parameters to story variants.The story components lack descriptive documentation. Adding parameters with descriptions would help users understand the purpose of each variant.
export const Horizontal = { + parameters: { + docs: { + description: { + story: 'Horizontal navigation using left/right arrow keys' + } + } + }, args: { direction: 'horizontal', loop: true } }; export const Vertical = { + parameters: { + docs: { + description: { + story: 'Vertical navigation using up/down arrow keys' + } + } + }, args: { direction: 'vertical', loop: true } };Also applies to: 46-58
15-43: Excellent demonstration of RovingFocusGroup capabilities.The stories provide comprehensive examples of:
- Different orientations (horizontal/vertical)
- Keyboard navigation behavior
- Handling of disabled items
- Accessibility features with ARIA attributes
This is a significant improvement over previous versions and will be very helpful for developers using this component.
Also applies to: 61-154, 157-256, 259-330
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/ui/Tabs/fragments/TabTrigger.tsx (1)
39-47: Add explicit button type for clarity.Wrapping the tab trigger in
<RovingFocusGroup.Item>is a solid approach for roving focus. However, adding atype="button"to the<button>element ensures it won’t inadvertently submit forms in nested contexts.<RovingFocusGroup.Item onFocus={() => handleFocus(tab)} > - <button + <button type="button" ref={ref} className={clsx(`${rootClass}-trigger`, `${isActive ? 'active' : ''}`, className)} role="tab" {...props} > {tab.label} </button> </RovingFocusGroup.Item>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/tools/SandboxEditor/SandboxEditor.tsx(4 hunks)src/components/ui/Tabs/fragments/TabTrigger.tsx(3 hunks)
🔇 Additional comments (7)
src/components/ui/Tabs/fragments/TabTrigger.tsx (2)
8-8: Good addition of the RovingFocusGroup import.Importing
RovingFocusGroupaligns with the new focus management approach in this pull request. This import is used effectively below, so no issues here.
22-22: Confirm removal of keyboard navigation logic.Previously, tab navigation might have relied on
previousTabornextTabfromTabsRootContext. Verify that removing these does not regress the keyboard navigation experience. If this logic has been superseded byRovingFocusGroup, no further action is required.src/components/tools/SandboxEditor/SandboxEditor.tsx (5)
1-1: Remove unused ‘forwardRef’ import.As noted in a previous review, this import is still unused.
-import React, { forwardRef, PropsWithChildren, useEffect, useState } from 'react'; +import React, { PropsWithChildren, useEffect, useState } from 'react';
7-7: No issues with importing RovingFocusGroup.This supports the new roving focus management.
27-31: Ensure semantic clarity for the color swatch.Turning
ColorSelectinto a<span>is acceptable since the focusable wrapper is provided externally. Just confirm thearia-labelis effectively read out when tabbing onto the outer<button>that now containsColorSelect.
59-59: Confirm design intent for changing the variant to "solid".Switching to a "solid" variant can affect the UI. Ensure this aligns with the dark-mode toggle design.
71-89: RovingFocusGroup usage looks correct.Wrapping color swatches in
RovingFocusGroup.Groupand usingRovingFocusGroup.Itemis a solid approach for keyboard navigation. TheonFocusandonClickboth set the color, which should improve accessibility.
RovingFocusGroup - adding this component in collaboration with @GoldGroove06 @jindaljyoti
TODO
Summary by CodeRabbit
New Features
Refactor