Tabs Improvements and Refactor #521
Conversation
WalkthroughThe changes in this pull request involve a significant restructuring of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #521 +/- ##
=======================================
Coverage 92.30% 92.30%
=======================================
Files 12 12
Lines 65 65
Branches 15 15
=======================================
Hits 60 60
Misses 5 5 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (12)
src/components/ui/Tabs/fragments/TabList.tsx (2)
Line range hint
10-16: Update TabListProps type to match component implementation.The type definition is out of sync with the actual component implementation:
- Missing
childrenprop that's used in the component- Contains unused props (
tabs,activeTab,setActiveTab,customRootClass)Apply this fix:
export type TabListProps = { - tabs?: Tab[] className?: string; - customRootClass?: string; - setActiveTab: React.Dispatch<Tab>; - activeTab: TabProps; + children: React.ReactNode; }
19-19: Add type safety for rootClass context value.The
rootClassfrom context should be validated to ensure it's not undefined, as this could lead to invalid class names.Consider adding a check:
- const { rootClass } = useContext(TabsRootContext); + const { rootClass } = useContext(TabsRootContext); + if (!rootClass) { + throw new Error('TabList must be used within a Tabs component'); + }src/components/ui/Tabs/Tabs.tsx (1)
Accessibility and state management implementation is correct, but keyboard navigation needs improvement
The implementation has proper accessibility attributes and state management, but there are a few issues to address:
TabTriggercorrectly implementsrole="tab", focus management, and tab indexing- State management through context is properly implemented
- However, keyboard navigation (
ArrowLeft/ArrowRight) only logs to console instead of actually changing focusSpecific locations needing attention:
src/components/ui/Tabs/fragments/TabTrigger.tsx: Lines 28-35 - Replaceconsole.log(tab)with actual tab activation in keyboard handlerssrc/components/ui/Tabs/fragments/TabContent.tsx: Missingrole="tabpanel"andaria-labelledbyattributes for better accessibility🔗 Analysis chain
Line range hint
1-37: Verify tab component usage patternsLet's ensure consistent implementation across the codebase.
Let me gather more information about the accessibility implementation and state management in the tab components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential accessibility issues in tab components echo "Checking for accessibility attributes in tab components..." rg -l "role=\"tab\"" src/ rg -l "aria-selected" src/ # Check for existing controlled component implementations echo "Checking for value prop usage patterns..." ast-grep --pattern 'const $_ = ({ value, $$$' # Look for potential tab state management patterns echo "Checking for tab state management patterns..." rg -l "activeTab|selectedTab|currentTab" src/Length of output: 703
Script:
#!/bin/bash # Check TabTrigger implementation for accessibility echo "TabTrigger implementation:" cat src/components/ui/Tabs/fragments/TabTrigger.tsx # Check TabRoot state management echo -e "\nTabRoot implementation:" cat src/components/ui/Tabs/fragments/TabRoot.tsx # Check TabContent implementation echo -e "\nTabContent implementation:" cat src/components/ui/Tabs/fragments/TabContent.tsxLength of output: 4716
styles/themes/components/tabs.scss (4)
3-5: Consider adding focus management styles.While the layout is good, consider enhancing accessibility by adding focus indicators and keyboard navigation styles.
.rad-ui-tabs-list{ display: flex; box-shadow: inset 0 -1px 0 0 var(--rad-ui-color-gray-500); + &:focus-within { + outline: 2px solid var(--rad-ui-color-accent-600); + outline-offset: -2px; + } }
8-28: Enhance state transitions and accessibility.Consider adding smooth transitions and respecting user motion preferences.
.rad-ui-tabs-trigger { color: var(--rad-ui-color-accent-600); font-family: inherit; padding: 10px 10px; display: flex; align-items: center; height:42px; font-size: 15px; line-height: 1; cursor: pointer; + transition: color 0.2s ease, border-color 0.2s ease; + + @media (prefers-reduced-motion: reduce) { + transition: none; + } + + &:focus-visible { + outline: 2px solid var(--rad-ui-color-accent-600); + outline-offset: -2px; + } &.active{ color: var(--rad-ui-color-accent-950); border-bottom:2px solid var(--rad-ui-color-accent-900); + aria-selected: true; }
31-34: Consider enhancing content area styling.The content area could benefit from improved spacing and transitions.
.rad-ui-tab-content{ - padding:20px 0px; + padding: var(--rad-ui-spacing-5) 0; color : var(--rad-ui-color-gray-1000); + transition: opacity 0.2s ease; + + @media (prefers-reduced-motion: reduce) { + transition: none; + } }
2-35: Consider documenting CSS custom properties.The styling uses various CSS custom properties (e.g.,
--rad-ui-color-accent-600). Consider adding documentation for these variables, either in comments or in a separate documentation file, to help maintain consistency across the project.src/components/ui/Tabs/fragments/TabRoot.tsx (1)
29-35: LGTM: Context values are well-structuredThe context provides all necessary values for tab functionality. Consider adding TypeScript interface for better type safety:
interface TabsContextValue { rootClass: string; activeTab: string; setActiveTab: (tab: string) => void; nextTab: () => void; previousTab: () => void; tabs: Array<{ value: string; [key: string]: any }>; }src/components/ui/Tabs/fragments/TabTrigger.tsx (4)
Line range hint
8-15: Update TabTriggerProps interface to match implementationThe TabTriggerProps interface contains props that are no longer used in the component implementation. This mismatch could cause confusion and TypeScript errors.
Apply this diff to align the interface with the implementation:
export type TabTriggerProps = { tab: TabProps; - setActiveTab: React.Dispatch<Tab>; - activeTab: TabProps; className?: string; - customRootClass?: string; - index: number; - props?: Record<string, any>[] + [key: string]: any; }
18-24: Optimize context usage and prop handling
- The
tabsvalue is destructured from context but never used- Consider memoizing the component to prevent unnecessary re-renders
Apply these optimizations:
-const { tabs, previousTab, nextTab, activeTab, setActiveTab, rootClass } = useContext(TabsRootContext); +const { previousTab, nextTab, activeTab, setActiveTab, rootClass } = useContext(TabsRootContext); +const TabTrigger = React.memo(({ tab, className = '', ...props }: TabTriggerProps) => {
40-47: Document the focus management patternThe comments describe an important pattern for focus management, but this should be properly documented for future reference.
Consider adding a JSDoc comment:
+/** + * Handles focus events on the tab trigger. + * Sets the active tab state based on focus, implementing a pattern where + * focus state drives the active tab state instead of managing it separately + * in the parent component. + */ const handleFocus = (tab: TabProps) => {
Add missing ARIA attributes for tab accessibility
The current implementation needs additional ARIA attributes to fully comply with WCAG requirements for tabs:
- Add
aria-selected={isActive}to indicate the selected state- Add
aria-controlsto associate the tab with its panel- Add
aria-labelledbyto associate the panel with its tabThe keyboard navigation is implemented correctly with arrow keys and focus management, but these ARIA attributes are essential for screen readers to properly announce the tab states and relationships.
Example locations needing updates:
src/components/ui/Tabs/fragments/TabTrigger.tsx: Add missing ARIA attributes to the button elementsrc/components/ui/Tabs/fragments/TabPanel.tsx: Add corresponding ARIA attributes for the panel component🔗 Analysis chain
Line range hint
1-63: Verify accessibility requirementsWhile the changes improve accessibility, let's ensure we meet all WCAG requirements for tabs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other tab implementations to ensure consistent accessibility patterns # Look for aria-* attributes in other components rg "aria-(selected|controls|labelledby)" --type tsx # Look for role="tab" usage rg 'role="tab"' --type tsx # Look for keyboard handling in other tab components ast-grep --pattern 'onKeyDown = {$_}'Length of output: 178
Script:
#!/bin/bash # Search for other tab implementations to ensure consistent accessibility patterns # Using more generic file extensions # Look for aria-* attributes in other components rg "aria-(selected|controls|labelledby)" -t ts -t tsx -t jsx # Look for role="tab" usage rg 'role="tab"' -t ts -t tsx -t jsx # Look for keyboard handling in other tab components rg "onKeyDown" -t ts -t tsx -t jsxLength of output: 219
Script:
#!/bin/bash # Search for other tab implementations to ensure consistent accessibility patterns # Using just pattern search without file type filters # Look for aria-* attributes in other components rg "aria-(selected|controls|labelledby)" # Look for role="tab" usage rg 'role="tab"' # Look for keyboard handling in other tab components rg "onKeyDown" # Also search for any accessibility-related documentation rg -l "accessibility|WCAG|a11y"Length of output: 1159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/components/ui/Tabs/Tabs.tsx(2 hunks)src/components/ui/Tabs/fragments/TabList.tsx(2 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(1 hunks)src/components/ui/Tabs/fragments/TabTrigger.tsx(2 hunks)styles/themes/components/tabs.scss(1 hunks)
🔇 Additional comments (4)
src/components/ui/Tabs/Tabs.tsx (1)
5-5: LGTM: Import follows established pattern
The TabTrigger import follows the same fragment-based architecture as other tab components.
styles/themes/components/tabs.scss (1)
2-2: LGTM! Improved class naming.
The rename from .rad-ui-tab-root to .rad-ui-tabs provides a more semantic and consistent naming convention.
src/components/ui/Tabs/fragments/TabRoot.tsx (2)
2-2: LGTM: Import changes are appropriate
The new imports support the batch-based navigation system and ref-based DOM manipulation.
Also applies to: 6-6
18-27: 🛠️ Refactor suggestion
Add error handling and verify keyboard navigation
While the focus management is good for accessibility, there are some concerns:
- Missing error handling for null refs
- Need to verify keyboard navigation implementation
Add error handling:
const nextTab = () => {
+ if (!tabRef?.current) return;
const batches = getAllBatchElements(tabRef.current);
+ if (!batches.length) return;
const nextItem = getNextBatchItem(batches);
nextItem.focus();
};| const TabList = ({ className = '', children }: TabListProps) => { | ||
| const { rootClass } = useContext(TabsRootContext); | ||
| return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={`${rootClass}-list ${className}`}> | ||
| {children} | ||
| </div>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with keyboard navigation support.
The tablist implementation should include keyboard navigation support as per WAI-ARIA practices:
- Left/Right arrows for horizontal navigation
- Home/End keys for first/last tab
Consider implementing keyboard event handlers:
const TabList = ({ className = '', children }: TabListProps) => {
const { rootClass } = useContext(TabsRootContext);
+ const handleKeyDown = (event: React.KeyboardEvent) => {
+ // Handle arrow keys for navigation
+ switch (event.key) {
+ case 'ArrowRight':
+ case 'ArrowLeft':
+ event.preventDefault();
+ // Navigate between tabs
+ break;
+ case 'Home':
+ case 'End':
+ event.preventDefault();
+ // Navigate to first/last tab
+ break;
+ }
+ };
+
return <div
role="tablist"
aria-orientation='horizontal'
aria-label="todo"
+ onKeyDown={handleKeyDown}
className={`${rootClass}-list ${className}`}>
{children}
</div>;
};Committable suggestion skipped: line range outside the PR's diff.
| return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={`${rootClass}-list ${className}`}> | ||
| {children} |
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the Tabs component files, I can now generate the final response since we have sufficient context. The TabList component is part of a well-structured tabs implementation that includes proper keyboard navigation and ARIA attributes, but the aria-label is indeed a placeholder that needs to be derived from the tabs' context.
The aria-label should be derived from the tabs prop
The "todo" placeholder should be replaced with a descriptive label based on the tabs' context since:
- The component receives
tabsthrough context which contains meaningful information - Other accessibility features (keyboard navigation, roles, orientation) are properly implemented
- This is the only instance of a placeholder aria-label in the codebase
Consider updating the TabList component to:
aria-label={`${tabs.length} tabs`} // or derive from tabs content🔗 Analysis chain
Replace placeholder aria-label with descriptive text.
The "todo" aria-label is not descriptive and needs to be replaced with meaningful text that describes the purpose of this tablist.
Let's check if this is a recurring issue:
Since we found only one instance of the placeholder "todo" aria-label in the TabList component, and no other occurrences in the codebase, let's verify if there are any other similar patterns or if aria-labels are properly used elsewhere in the Tabs component for better context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other placeholder aria-labels
rg -g '*.{tsx,jsx}' 'aria-label="todo"'
Length of output: 211
Script:
#!/bin/bash
# Search for aria-label usage in Tabs component files
fd . src/components/ui/Tabs -e tsx -e jsx --exec rg -l "aria-label"
# Show the content of these files to understand the aria-label patterns
fd . src/components/ui/Tabs -e tsx -e jsx --exec cat {}
Length of output: 6828
| @@ -14,12 +15,16 @@ export type TabsProps = { | |||
|
|
|||
| const Tabs = ({ tabs = [], ...props }: TabsProps) => { | |||
| // This should be a value <`tabs.value`> that is passed in from the parent component | |||
| const [activeTab, setActiveTab] = useState(tabs[0].value || ''); | |||
|
|
|||
| const defaultActiveTab = tabs[0].value || ''; | |||
There was a problem hiding this comment.
Add null check for empty tabs array
The current implementation could throw an error if tabs array is empty.
Add proper fallback:
- const defaultActiveTab = tabs[0].value || '';
+ const defaultActiveTab = tabs[0]?.value ?? '';📝 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.
| const defaultActiveTab = tabs[0].value || ''; | |
| const defaultActiveTab = tabs[0]?.value ?? ''; |
| <TabList> | ||
| {tabs.map((tab) => ( | ||
| <TabTrigger key={tab.value} tab={tab} /> | ||
| ))} | ||
| </TabList> |
There was a problem hiding this comment.
Enhance accessibility attributes for tab navigation
Given the PR's focus on accessibility improvements, the TabList implementation should include proper ARIA attributes.
Add necessary accessibility attributes:
- <TabList>
+ <TabList
+ role="tablist"
+ aria-label="Navigation tabs"
+ >
{tabs.map((tab) => (
- <TabTrigger key={tab.value} tab={tab} />
+ <TabTrigger
+ key={tab.value}
+ tab={tab}
+ role="tab"
+ aria-selected={tab.value === activeTab}
+ aria-controls={`panel-${tab.value}`}
+ />
))}
</TabList>📝 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.
| <TabList> | |
| {tabs.map((tab) => ( | |
| <TabTrigger key={tab.value} tab={tab} /> | |
| ))} | |
| </TabList> | |
| <TabList | |
| role="tablist" | |
| aria-label="Navigation tabs" | |
| > | |
| {tabs.map((tab) => ( | |
| <TabTrigger | |
| key={tab.value} | |
| tab={tab} | |
| role="tab" | |
| aria-selected={tab.value === activeTab} | |
| aria-controls={`panel-${tab.value}`} | |
| /> | |
| ))} | |
| </TabList> |
|
|
||
| const defaultActiveTab = tabs[0].value || ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing controlled state pattern
The comment suggests external control capability, but the implementation only supports uncontrolled state via defaultActiveTab.
Consider adding controlled state support:
const Tabs = ({ tabs = [], ...props }: TabsProps) => {
- // This should be a value <`tabs.value`> that is passed in from the parent component
- const defaultActiveTab = tabs[0].value || '';
+ const [activeTab, setActiveTab] = useState(props.value ?? tabs[0]?.value ?? '');
+
+ // Handle external updates when used as controlled component
+ React.useEffect(() => {
+ if (props.value !== undefined) {
+ setActiveTab(props.value);
+ }
+ }, [props.value]);Committable suggestion skipped: line range outside the PR's diff.
| const tabRef = useRef(null); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety for the ref and handle edge cases
The ref lacks type annotation and the component might break with an empty tabs array.
Consider these improvements:
- const tabRef = useRef(null);
+ const tabRef = useRef<HTMLDivElement>(null);Also, consider adding validation:
if (!tabs.length) {
console.warn('TabRoot: No tabs provided');
}| }}> | ||
| <div className={`${rootClass} ${className}`} data-accent-color={color} {...props} > | ||
| value={contextValues}> | ||
| <div ref={tabRef} className={`${rootClass} ${className}`} data-accent-color={color} {...props} > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for better accessibility
While the structure is good, consider adding ARIA attributes to improve accessibility.
- <div ref={tabRef} className={`${rootClass} ${className}`} data-accent-color={color} {...props} >
+ <div
+ ref={tabRef}
+ className={`${rootClass} ${className}`}
+ data-accent-color={color}
+ role="tablist"
+ aria-orientation="horizontal"
+ {...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={tabRef} className={`${rootClass} ${className}`} data-accent-color={color} {...props} > | |
| <div | |
| ref={tabRef} | |
| className={`${rootClass} ${className}`} | |
| data-accent-color={color} | |
| role="tablist" | |
| aria-orientation="horizontal" | |
| {...props} | |
| > |
| <button | ||
| ref={ref} | ||
| role="tab" key={index} className={`${rootClass} ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | ||
| onClick={() => handleClick(tab)}> | ||
| <span className={`${rootClass}-inner`}> | ||
| {tab.label} | ||
| </span> | ||
| role="tab" className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | ||
| onClick={() => handleClick(tab)} | ||
| onFocus={() => handleFocus(tab)} | ||
| tabIndex={isActive ? 0 : -1} | ||
| data-rad-ui-batch-element | ||
| > | ||
| {tab.label} | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and className handling
The button element could benefit from additional ARIA attributes and better className organization.
Apply these improvements:
<button
ref={ref}
role="tab"
- className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`}
+ className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`.trim()}
{...props}
onKeyDown={handleKeyDownEvent}
onClick={() => handleClick(tab)}
onFocus={() => handleFocus(tab)}
tabIndex={isActive ? 0 : -1}
+ aria-selected={isActive}
+ aria-controls={`${tab.value}-panel`}
data-rad-ui-batch-element
>📝 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 | |
| ref={ref} | |
| role="tab" key={index} className={`${rootClass} ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)}> | |
| <span className={`${rootClass}-inner`}> | |
| {tab.label} | |
| </span> | |
| role="tab" className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)} | |
| onFocus={() => handleFocus(tab)} | |
| tabIndex={isActive ? 0 : -1} | |
| data-rad-ui-batch-element | |
| > | |
| {tab.label} | |
| </button> | |
| ); | |
| <button | |
| ref={ref} | |
| role="tab" | |
| className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`.trim()} | |
| {...props} | |
| onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)} | |
| onFocus={() => handleFocus(tab)} | |
| tabIndex={isActive ? 0 : -1} | |
| aria-selected={isActive} | |
| aria-controls={`${tab.value}-panel`} | |
| data-rad-ui-batch-element | |
| > | |
| {tab.label} | |
| </button> | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/ui/Tabs/fragments/TabTrigger.tsx (3)
Line range hint
8-16: Update TabTriggerProps type definition to match actual usageThe type definition includes props that are no longer directly passed to the component as they're now handled through context.
Apply this fix:
export type TabTriggerProps = { tab: TabProps; - setActiveTab: React.Dispatch<Tab>; - activeTab: TabProps; className?: string; - customRootClass?: string; - index: number; props?: Record<string, any>[] }
44-46: Simplify code commentsWhile the implementation is good, the comments are overly verbose and repeat the same information.
Replace with a single concise comment:
- // 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 + // Update active tab on focus to maintain tab state consistency
Line range hint
1-62: Consider comprehensive accessibility implementationWhile the component has been refactored successfully, consider implementing a complete accessibility solution following the WAI-ARIA Tabs Pattern. This should include:
- Keyboard interaction for all tab operations
- Proper ARIA roles, states, and properties
- Focus management
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/ui/Tabs/fragments/TabTrigger.tsx(3 hunks)
🔇 Additional comments (3)
src/components/ui/Tabs/fragments/TabTrigger.tsx (3)
18-21: LGTM! Clean component signature and context usage
The simplified props and expanded context usage improve the component's maintainability.
Line range hint 30-36: Fix keyboard navigation implementation
The keyboard navigation handlers still don't update the active tab state.
51-57: Enhance accessibility attributes
The button element still needs additional ARIA attributes for better accessibility.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
src/components/ui/Tabs/fragments/TabTrigger.tsx (1)
Line range hint
8-16: Update TabTriggerProps interface to match implementationThe interface includes unused props that are now handled through context. This mismatch could cause confusion and TypeScript errors.
Apply this update:
export type TabTriggerProps = { tab: TabProps; - setActiveTab: React.Dispatch<Tab>; - activeTab: TabProps; className?: string; - customRootClass?: string; - index: number; props?: Record<string, any>[] }src/components/ui/Tabs/stories/Tabs.stories.js (1)
51-68: Enhance accessibility and consistency in tab configuration.The story configuration has a few areas for improvement:
- ArrowIcon in tab label needs accessibility attributes
- Content formatting could be more consistent
{ label: <div className='flex items-center space-x-2'> - <span>Tab Out</span> <ArrowIcon/> + <span>Tab Out</span> + <ArrowIcon aria-hidden="true" className="ml-1" /> </div>, value: 'tab_out_2', - content: <div className='text-gray-1000'> + content: <div className="text-gray-1000 space-y-2"> Once you tab out of the Tab component, you will be focused on the second button. </div> }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/ui/Tabs/fragments/TabTrigger.tsx(3 hunks)src/components/ui/Tabs/stories/Tabs.stories.js(2 hunks)
🔇 Additional comments (4)
src/components/ui/Tabs/fragments/TabTrigger.tsx (2)
18-23: LGTM! Clean context usage and initialization
The component properly utilizes context and follows React best practices with clean destructuring and proper state management.
Line range hint 18-60: Verify TabTrigger usage across the codebase
Let's ensure the component is being used correctly with the new context-based implementation.
✅ Verification successful
TabTrigger implementation verified and properly used
The verification shows that TabTrigger is being used correctly with the context-based implementation:
- TabTrigger is only imported and used within the Tabs component hierarchy
- No direct prop passing of
setActiveTaboractiveTabto TabTrigger components - The component is properly used with just the required
tabprop as seen inTabs.tsx:<TabTrigger key={tab.value} tab={tab} />
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct TabTrigger usage to verify proper implementation
rg -l "TabTrigger" --type tsx
# Check for any remaining direct prop passing that should be removed
rg "setActiveTab={|activeTab={|customRootClass={|index={" --type tsx
Length of output: 277
Script:
#!/bin/bash
# Search for TabTrigger usage without file type restriction
rg -l "TabTrigger"
# Check for prop passing patterns individually
rg "setActiveTab={"
rg "activeTab={"
rg "customRootClass={"
rg "index={"
# Search for imports of TabTrigger
rg "import.*TabTrigger"
Length of output: 769
Script:
#!/bin/bash
# Let's check the actual usage patterns in the files
rg "TabTrigger" -A 5 -B 5 src/components/ui/Tabs/Tabs.tsx
rg "TabTrigger" -A 5 -B 5 src/components/ui/Tabs/fragments/TabList.tsx
# Search for prop patterns with escaped curly braces
rg "setActiveTab=\{"
rg "activeTab=\{"
rg "customRootClass=\{"
Length of output: 2039
src/components/ui/Tabs/stories/Tabs.stories.js (2)
4-4: LGTM!
The Button import is correctly added and will be used to demonstrate tab navigation behavior.
51-68: Verify tab navigation behavior matches the refactored implementation.
Since this story is part of a larger tabs refactor that changed from activeTab to defaultActiveTab, we should verify that the navigation behavior works as expected.
✅ Verification successful
Tab navigation implementation is correctly implemented and matches the story's description
The TabInTabOut story correctly demonstrates the refactored tab behavior because:
- The implementation uses
defaultTabprop as shown in TabRoot component - TabTrigger component properly handles keyboard navigation with:
- Arrow key navigation between tabs using
previousTabandnextTab - Correct tab focus management with
tabIndexandonFocushandlers
- Arrow key navigation between tabs using
- The story's setup with buttons before and after the Tabs component allows testing the tab-in and tab-out behavior as described in the content
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of defaultActiveTab prop across stories and tests
# Look for any remaining activeTab references that might need updating
rg -l "activeTab" --type js --type jsx --type tsx
# Check for defaultActiveTab usage in test files
rg -l "defaultActiveTab" --type test
Length of output: 143
Script:
#!/bin/bash
# Search for activeTab and defaultActiveTab usage with correct file extensions
rg -l "activeTab" "*.js" "*.jsx" "*.tsx"
# Find test files containing defaultActiveTab
fd -e test.js -e test.jsx -e test.tsx -x rg -l "defaultActiveTab" {}
# Look specifically at the Tabs component implementation
fd -e js -e jsx -e tsx "Tabs" -x cat {}
Length of output: 5768
Script:
#!/bin/bash
# Look for TabRoot implementation to verify tab behavior
fd -e js -e jsx -e tsx "TabRoot" -x cat {}
# Check for any tab-related test files
fd "Tab.*test" -e js -e jsx -e tsx -x cat {}
# Look for any keyboard navigation related code in fragments
fd -e js -e jsx -e tsx -p "fragments" | grep -i "tab" | xargs cat
Length of output: 8931
| const handleFocus = (tab: TabProps) => { | ||
| if (ref.current) { | ||
| 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 | ||
| }; |
There was a problem hiding this comment.
Potential focus management issue
The handleFocus function forces focus on every focus event, which could interfere with natural keyboard navigation and create focus trapping issues.
Consider removing the programmatic focus setting:
const handleFocus = (tab: TabProps) => {
- if (ref.current) {
- ref.current?.focus();
- }
setActiveTab(tab.value);
}Format documentation as JSDoc comment
The implementation notes would be more useful as a proper JSDoc comment.
- // 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
+ /**
+ * Sets active tab on focus, eliminating need for parent state management.
+ * This pattern should be followed for similar component implementations.
+ */📝 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.
| const handleFocus = (tab: TabProps) => { | |
| if (ref.current) { | |
| 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 | |
| }; | |
| /** | |
| * Sets active tab on focus, eliminating need for parent state management. | |
| * This pattern should be followed for similar component implementations. | |
| */ | |
| const handleFocus = (tab: TabProps) => { | |
| setActiveTab(tab.value); | |
| }; |
| <button | ||
| ref={ref} | ||
| role="tab" key={index} className={`${rootClass} ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | ||
| onClick={() => handleClick(tab)}> | ||
| <span className={`${rootClass}-inner`}> | ||
| {tab.label} | ||
| </span> | ||
| role="tab" className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | ||
| onClick={() => handleClick(tab)} | ||
| onFocus={() => handleFocus(tab)} | ||
| tabIndex={isActive ? 0 : -1} | ||
| data-rad-ui-batch-element | ||
|
|
||
| > | ||
| {tab.label} | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button accessibility and formatting
The button element needs accessibility improvements and better formatting.
Apply these improvements:
<button
ref={ref}
role="tab"
- className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent}
- onClick={() => handleClick(tab)}
- onFocus={() => handleFocus(tab)}
- tabIndex={isActive ? 0 : -1}
- data-rad-ui-batch-element
+ className={[
+ `${rootClass}-trigger`,
+ isActive && 'active',
+ className
+ ].filter(Boolean).join(' ')}
+ aria-selected={isActive}
+ aria-controls={`${tab.value}-panel`}
+ id={`${tab.value}-tab`}
+ tabIndex={isActive ? 0 : -1}
+ data-rad-ui-batch-element
+ {...props}
+ onKeyDown={handleKeyDownEvent}
+ onClick={() => handleClick(tab)}
+ onFocus={() => handleFocus(tab)}
>
{tab.label}
</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 | |
| ref={ref} | |
| role="tab" key={index} className={`${rootClass} ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)}> | |
| <span className={`${rootClass}-inner`}> | |
| {tab.label} | |
| </span> | |
| role="tab" className={`${rootClass}-trigger ${isActive ? 'active' : ''} ${className}`} {...props} onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)} | |
| onFocus={() => handleFocus(tab)} | |
| tabIndex={isActive ? 0 : -1} | |
| data-rad-ui-batch-element | |
| > | |
| {tab.label} | |
| </button> | |
| <button | |
| ref={ref} | |
| role="tab" | |
| className={[ | |
| `${rootClass}-trigger`, | |
| isActive && 'active', | |
| className | |
| ].filter(Boolean).join(' ')} | |
| aria-selected={isActive} | |
| aria-controls={`${tab.value}-panel`} | |
| id={`${tab.value}-tab`} | |
| tabIndex={isActive ? 0 : -1} | |
| data-rad-ui-batch-element | |
| {...props} | |
| onKeyDown={handleKeyDownEvent} | |
| onClick={() => handleClick(tab)} | |
| onFocus={() => handleFocus(tab)} | |
| > | |
| {tab.label} | |
| </button> |
| const TabInTabOutTemplate = (args) => { | ||
| return <SandboxEditor> | ||
| <Button>Click me</Button> | ||
| <Tabs {...args} /> | ||
| <Button>Click me</Button> | ||
| </SandboxEditor>; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to Button components.
The buttons should have descriptive labels and ARIA attributes to improve accessibility, especially since they're being used to demonstrate tab navigation.
- <Button>Click me</Button>
+ <Button aria-label="First focusable button">Previous Element</Button>
- <Tabs {...args} />
+ <Tabs {...args} />
- <Button>Click me</Button>
+ <Button aria-label="Last focusable button">Next Element</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.
| const TabInTabOutTemplate = (args) => { | |
| return <SandboxEditor> | |
| <Button>Click me</Button> | |
| <Tabs {...args} /> | |
| <Button>Click me</Button> | |
| </SandboxEditor>; | |
| }; | |
| const TabInTabOutTemplate = (args) => { | |
| return <SandboxEditor> | |
| <Button aria-label="First focusable button">Previous Element</Button> | |
| <Tabs {...args} /> | |
| <Button aria-label="Last focusable button">Next Element</Button> | |
| </SandboxEditor>; | |
| }; |
This PR improves the way tabs work, more accessible and major refactoring done on the components
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style