-
-
Notifications
You must be signed in to change notification settings - Fork 53
Tabs Improvements and Refactor #521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
920c947
ee6454a
86a97e7
87ed313
e9f847f
1694f8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||||||||||||||||||||||||||||||||||||
| import React, { useState } from 'react'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| import TabList from './fragments/TabList'; | ||||||||||||||||||||||||||||||||||||||||
| import TabTrigger from './fragments/TabTrigger'; | ||||||||||||||||||||||||||||||||||||||||
| import TabContent from './fragments/TabContent'; | ||||||||||||||||||||||||||||||||||||||||
| import TabRoot from './fragments/TabRoot'; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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 || ''; | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+18
to
19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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]);
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||
| <TabRoot tabs={tabs} defaultTab={defaultActiveTab} > | ||||||||||||||||||||||||||||||||||||||||
| <TabList /> | ||||||||||||||||||||||||||||||||||||||||
| <TabList> | ||||||||||||||||||||||||||||||||||||||||
| {tabs.map((tab) => ( | ||||||||||||||||||||||||||||||||||||||||
| <TabTrigger key={tab.value} tab={tab} /> | ||||||||||||||||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||||||||||||||||
| </TabList> | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+23
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||
| <TabContent /> | ||||||||||||||||||||||||||||||||||||||||
| </TabRoot> | ||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| 'use client'; | ||
| import React, { useContext } from 'react'; | ||
| import { customClassSwitcher } from '~/core'; | ||
|
|
||
| import TabTrigger from './TabTrigger'; | ||
| import { TabProps } from '../types'; | ||
| import TabsRootContext from '../context/TabsRootContext'; | ||
|
|
@@ -15,21 +15,10 @@ export type TabListProps = { | |
| activeTab: TabProps; | ||
| } | ||
|
|
||
| const TabList = ({ className = '', customRootClass = '' }: TabListProps) => { | ||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||
| const { tabs, activeTab, setActiveTab } = useContext(TabsRootContext); | ||
|
|
||
| // TODO: in the previous return value of | ||
| // {tabs.map((tab, index) => { | ||
| // return <TabTrigger activeTab={activeTab} setActiveTab={setActiveTab} key={index} tab={tab} index={index} />; | ||
| // }) | ||
| // idk if React changed anything, but because its mapped, it shouldve worked. maybe it is the wrong prop? look into it. | ||
| // ive temporarily added an unnecessary ' | ||
| return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={`${rootClass} ${className}`}> | ||
| {tabs.map((tab, index) => { | ||
| return <TabTrigger activeTab={activeTab} setActiveTab={setActiveTab} key={index} tab={tab} index={index} />; | ||
| }) | ||
| } | ||
| const TabList = ({ className = '', children }: TabListProps) => { | ||
| const { rootClass } = useContext(TabsRootContext); | ||
| return <div role="tablist" aria-orientation='horizontal' aria-label="todo" className={`${rootClass}-list ${className}`}> | ||
| {children} | ||
|
Comment on lines
+20
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 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:
Consider updating the TabList component to: aria-label={`${tabs.length} tabs`} // or derive from tabs content🔗 Analysis chainReplace 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 executedThe 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 |
||
| </div>; | ||
|
Comment on lines
+18
to
22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance accessibility with keyboard navigation support. The tablist implementation should include keyboard navigation support as per WAI-ARIA practices:
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>;
};
|
||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,44 +1,44 @@ | ||||||||||||||||||||
| 'use client'; | ||||||||||||||||||||
| import React, { useState } from 'react'; | ||||||||||||||||||||
| import React, { useState, useRef } from 'react'; | ||||||||||||||||||||
| import { customClassSwitcher } from '~/core'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import TabsRootContext from '../context/TabsRootContext'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { TabRootProps } from '../types'; | ||||||||||||||||||||
| import { getAllBatchElements, getNextBatchItem, getPrevBatchItem } from '~/core/batches'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const COMPONENT_NAME = 'Tabs'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const TabRoot = ({ children, defaultTab = '', customRootClass, tabs = [], className, color, ...props }: TabRootProps) => { | ||||||||||||||||||||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const tabRef = useRef(null); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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');
} |
||||||||||||||||||||
| const [activeTab, setActiveTab] = useState(defaultTab || tabs[0].value || ''); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const nextTab = () => { | ||||||||||||||||||||
| const currentIndex = tabs.findIndex((tab) => tab.value === activeTab); | ||||||||||||||||||||
| const nextIndex = currentIndex + 1; | ||||||||||||||||||||
| if (nextIndex < tabs.length) { | ||||||||||||||||||||
| setActiveTab(tabs[nextIndex].value); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| const batches = getAllBatchElements(tabRef?.current); | ||||||||||||||||||||
| const nextItem = getNextBatchItem(batches); | ||||||||||||||||||||
| nextItem.focus(); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const previousTab = () => { | ||||||||||||||||||||
| const currentIndex = tabs.findIndex((tab) => tab.value === activeTab); | ||||||||||||||||||||
| const previousIndex = currentIndex - 1; | ||||||||||||||||||||
| if (previousIndex >= 0) { | ||||||||||||||||||||
| setActiveTab(tabs[previousIndex].value); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| const batches = getAllBatchElements(tabRef?.current); | ||||||||||||||||||||
| const prevItem = getPrevBatchItem(batches); | ||||||||||||||||||||
| prevItem.focus(); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const contextValues = { | ||||||||||||||||||||
| rootClass, | ||||||||||||||||||||
| activeTab, | ||||||||||||||||||||
| setActiveTab, | ||||||||||||||||||||
| nextTab, | ||||||||||||||||||||
| previousTab, | ||||||||||||||||||||
| tabs | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return ( | ||||||||||||||||||||
| <TabsRootContext.Provider | ||||||||||||||||||||
| value={{ | ||||||||||||||||||||
| activeTab, | ||||||||||||||||||||
| setActiveTab, | ||||||||||||||||||||
| nextTab, | ||||||||||||||||||||
| previousTab, | ||||||||||||||||||||
| tabs | ||||||||||||||||||||
| }}> | ||||||||||||||||||||
| <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||
| {children} | ||||||||||||||||||||
| </div> | ||||||||||||||||||||
| </TabsRootContext.Provider> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,12 +1,10 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'use client'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { useContext, useEffect, useRef } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { customClassSwitcher } from '~/core'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import React, { useContext, useRef } from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { TabProps } from '../types'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import TabsRootContext from '../context/TabsRootContext'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const COMPONENT_NAME = 'TabTrigger'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export type TabTriggerProps = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tab: TabProps; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setActiveTab: React.Dispatch<Tab>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -17,28 +15,14 @@ export type TabTriggerProps = { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| props?: Record<string, any>[] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const TabTrigger = ({ tab, setActiveTab, activeTab, className, customRootClass, index, ...props }: TabTriggerProps) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const TabTrigger = ({ tab, className = '', ...props }: TabTriggerProps) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // use context | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { tabs, previousTab, nextTab } = useContext(TabsRootContext); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ref = useRef(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleFocusTabEvent = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // focus on the active tab | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (activeTab === tab.value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ref.current.focus(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handleFocusTabEvent(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| , [activeTab]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const rootClass = customClassSwitcher(customRootClass, COMPONENT_NAME); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { previousTab, nextTab, activeTab, setActiveTab, rootClass } = useContext(TabsRootContext); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ref = useRef<HTMLButtonElement>(null); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const isActive = activeTab === tab.value; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleClick = (tab: Tab) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const handleClick = (tab: TabProps) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setActiveTab(tab.value); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -51,14 +35,28 @@ const TabTrigger = ({ tab, setActiveTab, activeTab, className, customRootClass, | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+38
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential focus management issue The 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <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> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
50
to
60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||||||||||||||||||||
| import Tabs from '../Tabs'; | ||||||||||||||||||||||||||||||
| import SandboxEditor from '~/components/tools/SandboxEditor/SandboxEditor'; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 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/Tabs', | ||||||||||||||||||||||||||||||
|
|
@@ -37,3 +39,31 @@ export const All = { | |||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| const TabInTabOutTemplate = (args) => { | ||||||||||||||||||||||||||||||
| return <SandboxEditor> | ||||||||||||||||||||||||||||||
| <Button>Click me</Button> | ||||||||||||||||||||||||||||||
| <Tabs {...args} /> | ||||||||||||||||||||||||||||||
| <Button>Click me</Button> | ||||||||||||||||||||||||||||||
| </SandboxEditor>; | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| export const TabInTabOut = TabInTabOutTemplate.bind({}); | ||||||||||||||||||||||||||||||
| TabInTabOut.args = { | ||||||||||||||||||||||||||||||
| tabs: [{ | ||||||||||||||||||||||||||||||
| label: 'Tabbing In', | ||||||||||||||||||||||||||||||
| value: 'tab_in_1', | ||||||||||||||||||||||||||||||
| content: 'Focus on the first button, press tab to move to the Tab component, it tabs into the selected tab. ' | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||
| label: <div className='flex items-center space-x-2'> <span>Tab Out</span> <ArrowIcon/></div>, | ||||||||||||||||||||||||||||||
| value: 'tab_out_2', | ||||||||||||||||||||||||||||||
| content: <div className='text-gray-1000'> | ||||||||||||||||||||||||||||||
| Once you tab out of the Tab component, you will be focused on the second button. | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,41 +1,39 @@ | ||
| /** TABS */ | ||
| .rad-ui-tab-root{ | ||
| .rad-ui-tabs{ | ||
| .rad-ui-tabs-list{ | ||
| display: flex; | ||
| box-shadow: inset 0 -1px 0 0 var(--rad-ui-color-gray-500); | ||
|
|
||
| } | ||
| .rad-ui-tab-list{ | ||
| display: flex; | ||
| box-shadow: inset 0 -1px 0 0 var(--rad-ui-color-gray-500); | ||
| } | ||
|
|
||
| .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; | ||
|
|
||
| .rad-ui-tab-trigger.active{ | ||
| color: var(--rad-ui-color-accent-950); | ||
| border-bottom:2px solid var(--rad-ui-color-accent-900); | ||
| } | ||
| .rad-ui-tab-trigger { | ||
| color: var(--rad-ui-color-accent-600); | ||
| font-family: inherit; | ||
| padding: 10px 10px; | ||
|
|
||
| display: flex; | ||
| height:42px; | ||
| font-size: 15px; | ||
| line-height: 1; | ||
| cursor: pointer; | ||
| } | ||
| .rad-ui-tab-trigger:hover{ | ||
| color: var(--rad-ui-color-accent-900); | ||
| } | ||
| &.active{ | ||
| color: var(--rad-ui-color-accent-950); | ||
| border-bottom:2px solid var(--rad-ui-color-accent-900); | ||
| } | ||
|
|
||
| .rad-ui-tab-trigger-inner{ | ||
| padding:4px 8px; | ||
| } | ||
| .rad-ui-tab-trigger:hover > .rad-ui-tab-trigger-inner{ | ||
| background-color: var(--rad-ui-color-accent-200); | ||
| border-radius: 4px; | ||
| } | ||
| &:hover{ | ||
| color: var(--rad-ui-color-accent-900); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .rad-ui-tab-content{ | ||
| padding:20px 0px; | ||
| color : var(--rad-ui-color-gray-1000); | ||
| .rad-ui-tab-content{ | ||
| padding:20px 0px; | ||
| color : var(--rad-ui-color-gray-1000); | ||
| } | ||
|
|
||
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add null check for empty tabs array
The current implementation could throw an error if tabs array is empty.
Add proper fallback:
📝 Committable suggestion