Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/components/ui/Tabs/Tabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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 || '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
const defaultActiveTab = tabs[0].value || '';
const defaultActiveTab = tabs[0]?.value ?? '';

Comment on lines +18 to 19
Copy link
Contributor

Choose a reason for hiding this comment

The 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]);

Committable suggestion skipped: line range outside the PR's diff.


return (
<TabRoot tabs={tabs} defaultTab={defaultActiveTab} >
<TabList />
<TabList>
{tabs.map((tab) => (
<TabTrigger key={tab.value} tab={tab} />
))}
</TabList>
Comment on lines +23 to +27
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
<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>

<TabContent />
</TabRoot>
);
Expand Down
21 changes: 5 additions & 16 deletions src/components/ui/Tabs/fragments/TabList.tsx
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';
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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 tabs through 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

</div>;
Comment on lines +18 to 22
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • 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.

};

Expand Down
42 changes: 21 additions & 21 deletions src/components/ui/Tabs/fragments/TabRoot.tsx
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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} >
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
<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}
>

{children}
</div>
</TabsRootContext.Provider>
Expand Down
52 changes: 25 additions & 27 deletions src/components/ui/Tabs/fragments/TabTrigger.tsx
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>;
Expand All @@ -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);
};

Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
};


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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
<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>

);
};
Expand Down
30 changes: 30 additions & 0 deletions src/components/ui/Tabs/stories/Tabs.stories.js
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',
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
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>;
};


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>
}

]

};
66 changes: 32 additions & 34 deletions styles/themes/components/tabs.scss
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);
}

}



Loading