feat: Add new Tabs component#4062
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a Preview React compound Tabs component with model-driven selection, panels, keyboard and overflow behavior, accessibility attributes, public exports, tests, Storybook stories, and usage documentation. ChangesTabs Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant Tabs
participant useTabsModel
participant OverflowListModel
participant MenuModel
Consumer->>Tabs: render Tabs.List, Tabs.Item, and Tabs.Panel
Tabs->>useTabsModel: create model state and events
useTabsModel->>OverflowListModel: manage selection, focus, orientation, and overflow
useTabsModel->>MenuModel: expose hidden items as menu entries
MenuModel->>useTabsModel: report selected overflow item
useTabsModel->>Tabs: update selection and panel visibility
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new preview Tabs compound component to Canvas Kit, including Storybook documentation/examples and initial unit/spec coverage, in support of issue #3980 (Sana Canvas Tabs updates + new item variants).
Changes:
- Introduces
@workday/canvas-kit-preview-react/tabswithTabs,useTabsModel, and supporting subcomponents (List/Item/Panel/Overflow). - Adds Storybook MDX docs plus example stories (basic, overflow, RTL, icons, dynamic tabs, etc.) and Chromatic testing stories.
- Adds initial Testing Library specs for Tabs, TabList, Tab, and TabPanel.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/preview-react/tabs/stories/tsconfig.json | Storybook TS config for the Tabs stories folder. |
| modules/preview-react/tabs/stories/testing.stories.tsx | Visual/static state coverage for Chromatic/testing (variants, overflow, RTL). |
| modules/preview-react/tabs/stories/Tabs.stories.ts | Primary Storybook stories entry for the Tabs docs/examples. |
| modules/preview-react/tabs/stories/Tabs.mdx | MDX documentation page for preview Tabs usage and API/spec sections. |
| modules/preview-react/tabs/stories/examples/Basic.tsx | Basic usage examples for filled + outline variants. |
| modules/preview-react/tabs/stories/examples/DisabledTab.tsx | Demonstrates disabled tab item behavior. |
| modules/preview-react/tabs/stories/examples/Icons.tsx | Demonstrates Tabs.Item.Icon + Tabs.Item.Text usage. |
| modules/preview-react/tabs/stories/examples/NamedTabs.tsx | Demonstrates data-id based tab/panel pairing. |
| modules/preview-react/tabs/stories/examples/RightToLeft.tsx | RTL usage example via CanvasProvider dir="rtl". |
| modules/preview-react/tabs/stories/examples/OverflowTabs.tsx | Overflow menu usage example with dynamic items/model. |
| modules/preview-react/tabs/stories/examples/HoistedModel.tsx | Hoisted useTabsModel example with external event triggering. |
| modules/preview-react/tabs/stories/examples/SinglePanel.tsx | Demonstrates advanced “single panel” composition pattern. |
| modules/preview-react/tabs/stories/examples/AlternativeTabStop.tsx | Demonstrates disabling panel focusability via tabIndex={undefined}. |
| modules/preview-react/tabs/stories/examples/DynamicTabs.tsx | Demonstrates dynamically adding/removing tabs and overflow behavior. |
| modules/preview-react/tabs/spec/tsconfig.json | Spec TS config for the preview Tabs package. |
| modules/preview-react/tabs/spec/Tabs.spec.tsx | Baseline SSR + onSelect behavior tests. |
| modules/preview-react/tabs/spec/TabPanel.spec.tsx | Tabs.Panel role/content + verifyComponent coverage. |
| modules/preview-react/tabs/spec/TabList.spec.tsx | Tabs.List role/content + verifyComponent coverage. |
| modules/preview-react/tabs/spec/Tab.spec.tsx | Tabs.Item role/content + verifyComponent coverage. |
| modules/preview-react/tabs/README.md | Package README and Storybook docs link. |
| modules/preview-react/tabs/LICENSE | License file for the new preview package. |
| modules/preview-react/tabs/lib/useTabsModel.tsx | Tabs model built on collection + overflow + menu models, incl. variant and panel registration. |
| modules/preview-react/tabs/lib/Tabs.tsx | Container/compound component wiring subcomponents and overflow menu integration. |
| modules/preview-react/tabs/lib/TabsList.tsx | Tab list implementation with overflow measurement + touch scroll masking. |
| modules/preview-react/tabs/lib/TabsItem.tsx | Tab item implementation, styling, selection/roving focus/overflow measurement. |
| modules/preview-react/tabs/lib/TabsPanel.tsx | Tab panel implementation + panel registration and ARIA wiring. |
| modules/preview-react/tabs/lib/TabsPanels.tsx | Helper for rendering panels from dynamic items via render-prop. |
| modules/preview-react/tabs/lib/TabsOverflowButton.tsx | Overflow trigger button that targets the overflow list + menu model. |
| modules/preview-react/tabs/lib/TabsMenuPopper.tsx | Popper wrapper for the overflow menu. |
| modules/preview-react/tabs/index.ts | Public exports for the preview Tabs package. |
| modules/preview-react/index.ts | Re-exports tabs from the preview-react barrel. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const modality = useModalityType(); | ||
| console.log(model.state.variant); | ||
|
|
| @@ -0,0 +1,59 @@ | |||
| import {Meta, StoryObj} from '@storybook/react'; | |||
|
|
|||
| import {Tabs} from '@workday/canvas-kit-react/tabs'; | |||
| <SymbolDoc name="Tabs" fileName="/react/" /> | ||
|
|
||
| ## Specifications | ||
|
|
||
| <Specifications file="./cypress/component/Tabs.spec.tsx" name="Tabs" /> |
| View the | ||
| [documentation for Tabs](https://workday.github.io/canvas-kit/?path=/docs/components-containers-tabs--docs) | ||
| on Storybook. |
| * Optional id for the whole `Tabs` group. The `aria-controls` of the `Tab.Item` and `id` of the | ||
| * `Tab.Panel` will automatically derived from this id. If not provided, a unique id will be | ||
| * created. |
| /** | ||
| * The identifier of the tab. This identifier will be used in change events and for `initialTab`. | ||
| * Must match the `data-id` of the associated tab panel. If this property is not provided, it will | ||
| * default to a string representation of the the zero-based index of the Tab when it was | ||
| * initialized. |
| /** | ||
| * The identifier of the tab. This identifier will be used in change events and for `initialTab`. | ||
| * Must match the `data-id` of the associated tab item. If this property is not provided, it will | ||
| * default to a string representation of the the zero-based index of the Tab when it was | ||
| * initialized. |
| /** | ||
| * Part of the ARIA specification for tabs. By default, all `tabpanel` elements have a `tabIndex` | ||
| * of `0` which makes the whole content area receive focus. If you have a focusable item near the | ||
| * top of the tab panel content area, you may set `tabIndex` to `undefined` to prevent the tab | ||
| * panel element from receiving focus. Only do this is a child of the tab panel can receive focus. | ||
| */ |
| Third Tab | ||
| </Tabs.Item> | ||
| </Tabs.List> | ||
| <Tabs.Panel cs={{marginBlockStart: system.gap.lg}} hidden={undefined} id="mytab-panel"> |
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
issue3980-tabs
|
| Run status |
|
| Run duration | 02m 26s |
| Commit |
|
| Committer | Raisa Primerova |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
809
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.64%
|
|
|---|---|
|
|
1528
|
|
|
371
|
Accessibility
99.47%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
modules/preview-react/tabs/spec/Tabs.spec.tsx (2)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
containervariable.The
containerdestructured fromrenderis never used. You can safely remove it.♻️ Proposed refactor
- const {container} = render( - <Tabs onSelect={cb} initialSelectedIds={['first']}> + render( + <Tabs onSelect={cb} initialTab="first">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/preview-react/tabs/spec/Tabs.spec.tsx` around lines 27 - 28, Update the render call in the Tabs test to stop destructuring the unused container value; invoke render without assigning its return value while preserving the existing Tabs props and test behavior.
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the idiomatic
initialTabprop instead ofinitialSelectedIds.The
Tabscomponent definesinitialTabin its model config for initial selection, but this test usesinitialSelectedIds. For accurate testing of the public API surface, you should useinitialTab.♻️ Proposed refactor
- const {container} = render( - <Tabs onSelect={cb} initialSelectedIds={['first']}> + const {container} = render( + <Tabs onSelect={cb} initialTab="first"> <Tabs.List>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/preview-react/tabs/spec/Tabs.spec.tsx` around lines 25 - 35, Update the Tabs test setup in “should call "onSelect" when tab is selected” to use the public initialTab prop instead of initialSelectedIds, preserving the same initial “first” tab selection.modules/preview-react/tabs/stories/examples/SinglePanel.tsx (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix implicit any type error for object indexing.
Indexing the
contentsobject withmodel.state.selectedIds[0](which is astring) may result in a TypeScript compiler error, as the object only has specific literal keys (first,second,third). Consider casting the index tokeyof typeof contentsto ensure type safety.♻️ Proposed fix to ensure type safety
- {contents[model.state.selectedIds[0]]} + {contents[model.state.selectedIds[0] as keyof typeof contents]}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/preview-react/tabs/stories/examples/SinglePanel.tsx` at line 35, Update the contents lookup in the SinglePanel render expression to type the selected ID as keyof typeof contents before indexing, resolving the implicit-any error while preserving the existing selected-content behavior.modules/preview-react/tabs/lib/TabsItem.tsx (1)
250-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove leftover debug artifact.
♻️ Proposed fix
- console.log(model.state.variant);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/preview-react/tabs/lib/TabsItem.tsx` at line 250, Remove the leftover console.log debug statement from the TabsItem component, including the model.state.variant logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/preview-react/tabs/lib/TabsItem.tsx`:
- Line 132: Update the disabled-state selector in TabsItem styling to target
only elements whose aria-disabled attribute equals "true", while preserving the
existing :disabled and .disabled selectors.
- Around line 227-232: Update the `aria-selected` value in the returned tab
attributes so unselected tabs provide `undefined` rather than `false`, while
selected tabs remain `true`; preserve the existing `role`, `aria-controls`, and
other attribute behavior.
- Around line 254-259: Update the StyledTabItem prop order in TabsItem so
model.state.variant is applied before spreading elemProps, allowing an
explicitly provided variant to override the model fallback while preserving the
default when no override is supplied.
In `@modules/preview-react/tabs/lib/TabsList.tsx`:
- Around line 36-64: Update getScrollPercentage to detect the tabs container’s
RTL direction and normalize horizontal scrollLeft before calculating the
percentage, while preserving the existing vertical behavior. In the horizontal
overflow rendering around the start/end masks, swap the gradient direction for
RTL so the fade aligns with the correct edge; keep LTR positioning unchanged.
In `@modules/preview-react/tabs/lib/TabsPanel.tsx`:
- Around line 42-64: Update the localId initialization in TabsPanel so panels
without data-id or item?.id derive their positional fallback from
state.panelIndexRef.current during render, before useMountLayout runs. Preserve
explicit data-id and item IDs, and keep registration cleanup aligned with the
resolved fallback ID so SSR and first-render visibility/ARIA attributes are
correct.
In `@modules/preview-react/tabs/lib/useTabsModel.tsx`:
- Around line 119-127: Update the nonInteractiveIds configuration in
useTabsModel’s useMenuModel setup to preserve disabled state for overflowed
tabs: derive it from the IDs represented by overflowItems rather than filtering
against hiddenIds in a way that excludes them. Ensure disabled overflowed tabs
remain non-selectable while preserving the menu’s existing selection behavior.
In `@modules/preview-react/tabs/stories/testing.stories.tsx`:
- Around line 52-64: Wrap the conditional Tabs.Item instances in a Tabs.List
within the Tabs component in the ComponentStatesTable render callback. Preserve
the existing hasIcon, variant, and item content behavior while ensuring both
branches are children of Tabs.List.
---
Nitpick comments:
In `@modules/preview-react/tabs/lib/TabsItem.tsx`:
- Line 250: Remove the leftover console.log debug statement from the TabsItem
component, including the model.state.variant logging.
In `@modules/preview-react/tabs/spec/Tabs.spec.tsx`:
- Around line 27-28: Update the render call in the Tabs test to stop
destructuring the unused container value; invoke render without assigning its
return value while preserving the existing Tabs props and test behavior.
- Around line 25-35: Update the Tabs test setup in “should call "onSelect" when
tab is selected” to use the public initialTab prop instead of
initialSelectedIds, preserving the same initial “first” tab selection.
In `@modules/preview-react/tabs/stories/examples/SinglePanel.tsx`:
- Line 35: Update the contents lookup in the SinglePanel render expression to
type the selected ID as keyof typeof contents before indexing, resolving the
implicit-any error while preserving the existing selected-content behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b7f2ce41-067a-4720-a076-3221ab0275e8
📒 Files selected for processing (31)
modules/preview-react/index.tsmodules/preview-react/tabs/LICENSEmodules/preview-react/tabs/README.mdmodules/preview-react/tabs/index.tsmodules/preview-react/tabs/lib/Tabs.tsxmodules/preview-react/tabs/lib/TabsItem.tsxmodules/preview-react/tabs/lib/TabsList.tsxmodules/preview-react/tabs/lib/TabsMenuPopper.tsxmodules/preview-react/tabs/lib/TabsOverflowButton.tsxmodules/preview-react/tabs/lib/TabsPanel.tsxmodules/preview-react/tabs/lib/TabsPanels.tsxmodules/preview-react/tabs/lib/useTabsModel.tsxmodules/preview-react/tabs/spec/Tab.spec.tsxmodules/preview-react/tabs/spec/TabList.spec.tsxmodules/preview-react/tabs/spec/TabPanel.spec.tsxmodules/preview-react/tabs/spec/Tabs.spec.tsxmodules/preview-react/tabs/spec/tsconfig.jsonmodules/preview-react/tabs/stories/Tabs.mdxmodules/preview-react/tabs/stories/Tabs.stories.tsmodules/preview-react/tabs/stories/examples/AlternativeTabStop.tsxmodules/preview-react/tabs/stories/examples/Basic.tsxmodules/preview-react/tabs/stories/examples/DisabledTab.tsxmodules/preview-react/tabs/stories/examples/DynamicTabs.tsxmodules/preview-react/tabs/stories/examples/HoistedModel.tsxmodules/preview-react/tabs/stories/examples/Icons.tsxmodules/preview-react/tabs/stories/examples/NamedTabs.tsxmodules/preview-react/tabs/stories/examples/OverflowTabs.tsxmodules/preview-react/tabs/stories/examples/RightToLeft.tsxmodules/preview-react/tabs/stories/examples/SinglePanel.tsxmodules/preview-react/tabs/stories/testing.stories.tsxmodules/preview-react/tabs/stories/tsconfig.json
|
|
||
| // Using opacity token applied to container to achieve disabled state instead of 'disabled" color tokens for | ||
| // icon and text colors | ||
| '&:disabled, &.disabled, &[aria-disabled]': { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the attribute selector to prevent false-positive disabled states.
The [aria-disabled] CSS selector matches any value, including "false". If a developer explicitly passes aria-disabled={false} to indicate the tab is active/enabled, it will incorrectly receive disabled styling. Target the "true" value specifically.
🐛 Proposed fix
- '&:disabled, &.disabled, &[aria-disabled]': {
+ '&:disabled, &.disabled, &[aria-disabled="true"]': {📝 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.
| '&:disabled, &.disabled, &[aria-disabled]': { | |
| '&:disabled, &.disabled, &[aria-disabled="true"]': { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/TabsItem.tsx` at line 132, Update the
disabled-state selector in TabsItem styling to target only elements whose
aria-disabled attribute equals "true", while preserving the existing :disabled
and .disabled selectors.
| return { | ||
| type: 'button' as const, | ||
| role: 'tab' as const, | ||
| 'aria-selected': selected, | ||
| 'aria-controls': slugify(`tabpanel-${state.id}-${name}`), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align aria-selected behavior with the documented API contract.
The documentation for aria-selected explicitly states it should be "true or undefined and never false". However, the current logic passes boolean false, which React will render as aria-selected="false" in the DOM.
🐛 Proposed fix
return {
type: 'button' as const,
role: 'tab' as const,
- 'aria-selected': selected,
+ 'aria-selected': selected || undefined,
'aria-controls': slugify(`tabpanel-${state.id}-${name}`),
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| type: 'button' as const, | |
| role: 'tab' as const, | |
| 'aria-selected': selected, | |
| 'aria-controls': slugify(`tabpanel-${state.id}-${name}`), | |
| }; | |
| return { | |
| type: 'button' as const, | |
| role: 'tab' as const, | |
| 'aria-selected': selected || undefined, | |
| 'aria-controls': slugify(`tabpanel-${state.id}-${name}`), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/TabsItem.tsx` around lines 227 - 232, Update
the `aria-selected` value in the returned tab attributes so unselected tabs
provide `undefined` rather than `false`, while selected tabs remain `true`;
preserve the existing `role`, `aria-controls`, and other attribute behavior.
| <StyledTabItem | ||
| as={Element} | ||
| maxWidth={modality === 'touch' ? undefined : 280} | ||
| {...elemProps} | ||
| variant={model.state.variant} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Allow user-provided props to override the model fallback.
Since TabsItemProps documents variant as an acceptable prop, users should be able to override the group's default variant for a specific tab. Currently, variant={model.state.variant} appears after {...elemProps}, unconditionally overriding any variant the user provides.
Swap the order so the model state acts as a default, while respecting explicit user overrides.
🐛 Proposed fix
<StyledTabItem
as={Element}
maxWidth={modality === 'touch' ? undefined : 280}
- {...elemProps}
variant={model.state.variant}
+ {...elemProps}
>📝 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.
| <StyledTabItem | |
| as={Element} | |
| maxWidth={modality === 'touch' ? undefined : 280} | |
| {...elemProps} | |
| variant={model.state.variant} | |
| > | |
| <StyledTabItem | |
| as={Element} | |
| maxWidth={modality === 'touch' ? undefined : 280} | |
| variant={model.state.variant} | |
| {...elemProps} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/TabsItem.tsx` around lines 254 - 259, Update
the StyledTabItem prop order in TabsItem so model.state.variant is applied
before spreading elemProps, allowing an explicitly provided variant to override
the model fallback while preserving the default when no override is supplied.
| function getScrollPercentage(elem: HTMLDivElement, direction: 'vertical' | 'horizontal'): number { | ||
| if (direction === 'vertical') { | ||
| if (elem.scrollHeight > elem.clientHeight) { | ||
| const percentage = elem.scrollTop / (elem.scrollHeight - elem.clientHeight); | ||
| return percentage; | ||
| } else { | ||
| return -1; | ||
| } | ||
| } | ||
| if (elem.scrollWidth > elem.clientWidth) { | ||
| const percentage = elem.scrollLeft / (elem.scrollWidth - elem.clientWidth); | ||
| return percentage; | ||
| } else { | ||
| return -1; | ||
| } | ||
| } | ||
|
|
||
| function getScrollPosition( | ||
| elem: HTMLDivElement, | ||
| direction: 'vertical' | 'horizontal' | ||
| ): 'start' | 'middle' | 'end' | undefined { | ||
| const percentage = getScrollPercentage(elem, direction); | ||
| return percentage <= -1 | ||
| ? undefined | ||
| : percentage > 0.99 | ||
| ? 'end' | ||
| : percentage > 0.01 | ||
| ? 'middle' | ||
| : 'start'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant files and nearby context
git ls-files 'modules/preview-react/tabs/lib/*' 'modules/preview-react/tabs/**/*' | sed -n '1,200p'
printf '\n--- TabsList.tsx (outline) ---\n'
ast-grep outline modules/preview-react/tabs/lib/TabsList.tsx --view expanded || true
printf '\n--- TabsList.tsx (relevant lines) ---\n'
cat -n modules/preview-react/tabs/lib/TabsList.tsx | sed -n '1,220p'
printf '\n--- Search for RTL / scroll handling / gradients in tabs module ---\n'
rg -n --hidden --no-ignore-vcs -S "rtl|direction|scrollLeft|scrollRight|gradient|mask-image|maskImage|overflow" modules/preview-react/tabs -g '!**/*.map' -g '!**/dist/**' || trueRepository: Workday/canvas-kit
Length of output: 13332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any companion styles/components that may affect RTL overflow masks
fd -a -t f 'TabsList' modules/preview-react/tabs || true
fd -a -t f 'Tab' modules/preview-react/tabs/lib || trueRepository: Workday/canvas-kit
Length of output: 742
Account for RTL in tab overflow positioning
modules/preview-react/tabs/lib/TabsList.tsx:36-64, 133-143 — the horizontal scrollLeft calculation and the start/end masks are LTR-only. In dir="rtl", overflow state can land on the wrong side and the fade can hide the wrong edge. Normalize the horizontal offset and swap the gradient direction for RTL.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/TabsList.tsx` around lines 36 - 64, Update
getScrollPercentage to detect the tabs container’s RTL direction and normalize
horizontal scrollLeft before calculating the percentage, while preserving the
existing vertical behavior. In the horizontal overflow rendering around the
start/end masks, swap the gradient direction for RTL so the fade aligns with the
correct edge; keep LTR positioning unchanged.
| const [localId, setLocalId] = React.useState(elemProps['data-id'] || item?.id || ''); | ||
|
|
||
| useMountLayout(() => { | ||
| if (item) { | ||
| return; | ||
| } | ||
| const defaultId = state.panelIndexRef.current; | ||
| const itemId = localId || String(defaultId); | ||
| events.registerPanel({id: itemId, textValue: ''}); | ||
| setLocalId(itemId); | ||
|
|
||
| return () => { | ||
| events.unregisterPanel({id: itemId}); | ||
| }; | ||
| }); | ||
|
|
||
| return { | ||
| role: 'tabpanel' as const, | ||
| 'aria-labelledby': slugify(`${state.id}-${localId}`), | ||
| hidden: !!localId && localId !== state.selectedIds[0], | ||
| id: slugify(`tabpanel-${state.id}-${localId}`), | ||
| tabIndex: 0 as const, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby tab-related files.
git ls-files 'modules/preview-react/tabs/lib/*' 'modules/preview-react/tabs/*' | sed -n '1,200p'
echo '--- outline TabsPanel.tsx ---'
ast-grep outline modules/preview-react/tabs/lib/TabsPanel.tsx --view expanded || true
echo '--- TabsPanel.tsx lines ---'
wc -l modules/preview-react/tabs/lib/TabsPanel.tsx
cat -n modules/preview-react/tabs/lib/TabsPanel.tsx | sed -n '1,220p'Repository: Workday/canvas-kit
Length of output: 5042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect supporting tab state and hooks referenced by TabsPanel.
rg -n "useMountLayout|panelIndexRef|selectedIds|registerPanel|unregisterPanel|slugify" modules/preview-react/tabs -S
echo '--- file list around tabs lib ---'
git ls-files 'modules/preview-react/tabs/lib/*' | sed -n '1,200p'Repository: Workday/canvas-kit
Length of output: 2320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- TabsPanel.tsx ---'
sed -n '1,220p' modules/preview-react/tabs/lib/TabsPanel.tsx | nl -ba
echo '--- search for TabsPanel usage / data-id / panelIndexRef ---'
rg -n "TabsPanel|data-id|panelIndexRef|selectedIds|registerPanel|unregisterPanel|useMountLayout" modules/preview-react -SRepository: Workday/canvas-kit
Length of output: 218
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- useTabsModel.tsx ---'
sed -n '1,220p' modules/preview-react/tabs/lib/useTabsModel.tsx
echo '--- TabsPanels.tsx ---'
sed -n '1,220p' modules/preview-react/tabs/lib/TabsPanels.tsx
echo '--- TabsItem.tsx (id/aria-controls refs) ---'
sed -n '200,280p' modules/preview-react/tabs/lib/TabsItem.tsxRepository: Workday/canvas-kit
Length of output: 6830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ListRenderItemContext|createElemPropsHook|useMountLayout|panelIndexRef|selectedIds" modules/preview-react packages -SRepository: Workday/canvas-kit
Length of output: 4064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- useTabsModel.tsx ---'
cat -n modules/preview-react/tabs/lib/useTabsModel.tsx | sed -n '1,220p'
echo '--- TabsPanels.tsx ---'
cat -n modules/preview-react/tabs/lib/TabsPanels.tsx | sed -n '1,220p'
echo '--- TabsItem.tsx excerpt ---'
cat -n modules/preview-react/tabs/lib/TabsItem.tsx | sed -n '210,270p'Repository: Workday/canvas-kit
Length of output: 8132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "export .*ListRenderItemContext|ListRenderItemContext" . -SRepository: Workday/canvas-kit
Length of output: 1612
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect collection wrapper and any tests around panel ids / SSR-like behavior.
sed -n '1,220p' modules/preview-react/tabs/lib/TabsPanels.tsx | cat -n
echo '--- specs mentioning panel id / aria / hidden ---'
rg -n "tabpanel-|aria-labelledby|hidden:|selectedIds|data-id|Panel" modules/preview-react/tabs/spec modules/preview-react/tabs/stories -SRepository: Workday/canvas-kit
Length of output: 11589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ListRenderItemContext.Provider|Collection|renderItem|indexRef" modules/preview-react/tabs modules/preview-react/collection -SRepository: Workday/canvas-kit
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- TabsPanels.tsx ---'
cat -n modules/preview-react/tabs/lib/TabsPanels.tsx | sed -n '1,220p'
echo '--- Tabs.tsx ---'
cat -n modules/preview-react/tabs/lib/Tabs.tsx | sed -n '1,240p'Repository: Workday/canvas-kit
Length of output: 6465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "<Tabs\\.Panel|TabsPanel|TabsPanels" modules/preview-react/tabs/stories modules/preview-react/tabs/spec -SRepository: Workday/canvas-kit
Length of output: 6106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [
Path('modules/preview-react/tabs/lib/TabsPanels.tsx'),
Path('modules/preview-react/tabs/lib/useTabsModel.tsx'),
]:
print(f'--- {p} ---')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if i <= 220:
print(f'{i:4d}: {line}')
print()
PYRepository: Workday/canvas-kit
Length of output: 6138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- useListRenderItem.tsx ---'
cat -n modules/react/collection/lib/useListRenderItem.tsx | sed -n '1,220p'
echo '--- useListModel related ---'
rg -n "useListModel|indexRef|registerItem|unregisterItem|ListRenderItemContext" modules/react/collection/lib -SRepository: Workday/canvas-kit
Length of output: 7658
Generate the fallback panel id before the first render. This only affects plain Tabs.Panel children without data-id: they stay at localId === '' until useMountLayout, so server-rendered output gets empty/duplicate id/aria-labelledby values and all panels remain visible. Derive the positional id during render instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/TabsPanel.tsx` around lines 42 - 64, Update
the localId initialization in TabsPanel so panels without data-id or item?.id
derive their positional fallback from state.panelIndexRef.current during render,
before useMountLayout runs. Preserve explicit data-id and item IDs, and keep
registration cleanup aligned with the resolved fallback ID so SSR and
first-render visibility/ARIA attributes are correct.
| const menu = useMenuModel( | ||
| useMenuModel.mergeConfig(config.menuConfig, { | ||
| id: `menu-${model.state.id}`, | ||
| items: overflowItems, | ||
| nonInteractiveIds: state.nonInteractiveIds.filter(key => !state.hiddenIds.includes(key)), | ||
| onSelect(data) { | ||
| menu.events.hide(); | ||
| events.select(data); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve disabled state for overflowed tabs.
Line 123 filters out the hidden IDs that actually populate this menu. Consequently, disabled overflowed tabs become selectable, while the retained visible IDs are absent from overflowItems.
Proposed fix
- nonInteractiveIds: state.nonInteractiveIds.filter(key => !state.hiddenIds.includes(key)),
+ nonInteractiveIds: state.nonInteractiveIds.filter(key => state.hiddenIds.includes(key)),📝 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 menu = useMenuModel( | |
| useMenuModel.mergeConfig(config.menuConfig, { | |
| id: `menu-${model.state.id}`, | |
| items: overflowItems, | |
| nonInteractiveIds: state.nonInteractiveIds.filter(key => !state.hiddenIds.includes(key)), | |
| onSelect(data) { | |
| menu.events.hide(); | |
| events.select(data); | |
| }, | |
| const menu = useMenuModel( | |
| useMenuModel.mergeConfig(config.menuConfig, { | |
| id: `menu-${model.state.id}`, | |
| items: overflowItems, | |
| nonInteractiveIds: state.nonInteractiveIds.filter(key => state.hiddenIds.includes(key)), | |
| onSelect(data) { | |
| menu.events.hide(); | |
| events.select(data); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/lib/useTabsModel.tsx` around lines 119 - 127,
Update the nonInteractiveIds configuration in useTabsModel’s useMenuModel setup
to preserve disabled state for overflowed tabs: derive it from the IDs
represented by overflowItems rather than filtering against hiddenIds in a way
that excludes them. Ensure disabled overflowed tabs remain non-selectable while
preserving the menu’s existing selection behavior.
| {({hasIcon, variant, ...props}) => ( | ||
| <Tabs variant={variant}> | ||
| {hasIcon ? ( | ||
| <Tabs.Item {...props}> | ||
| <Tabs.Item.Icon icon={configureIcon} /> | ||
| <Tabs.Item.Text>Icon</Tabs.Item.Text> | ||
| </Tabs.Item> | ||
| ) : ( | ||
| <Tabs.Item {...props}>Tab</Tabs.Item> | ||
| )} | ||
| </Tabs> | ||
| )} | ||
| </ComponentStatesTable> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrap Tabs.Item with Tabs.List.
Tabs.Item is meant to be composed inside a Tabs.List component, which usually provides the necessary layout and ARIA attributes (such as role="tablist"). Rendering Tabs.Item directly inside Tabs may result in incorrect visual layout or accessibility structure in these test states.
♻️ Proposed fix to ensure correct structure
{({hasIcon, variant, ...props}) => (
<Tabs variant={variant}>
- {hasIcon ? (
- <Tabs.Item {...props}>
- <Tabs.Item.Icon icon={configureIcon} />
- <Tabs.Item.Text>Icon</Tabs.Item.Text>
- </Tabs.Item>
- ) : (
- <Tabs.Item {...props}>Tab</Tabs.Item>
- )}
+ <Tabs.List>
+ {hasIcon ? (
+ <Tabs.Item {...props}>
+ <Tabs.Item.Icon icon={configureIcon} />
+ <Tabs.Item.Text>Icon</Tabs.Item.Text>
+ </Tabs.Item>
+ ) : (
+ <Tabs.Item {...props}>Tab</Tabs.Item>
+ )}
+ </Tabs.List>
</Tabs>
)}📝 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.
| {({hasIcon, variant, ...props}) => ( | |
| <Tabs variant={variant}> | |
| {hasIcon ? ( | |
| <Tabs.Item {...props}> | |
| <Tabs.Item.Icon icon={configureIcon} /> | |
| <Tabs.Item.Text>Icon</Tabs.Item.Text> | |
| </Tabs.Item> | |
| ) : ( | |
| <Tabs.Item {...props}>Tab</Tabs.Item> | |
| )} | |
| </Tabs> | |
| )} | |
| </ComponentStatesTable> | |
| {({hasIcon, variant, ...props}) => ( | |
| <Tabs variant={variant}> | |
| <Tabs.List> | |
| {hasIcon ? ( | |
| <Tabs.Item {...props}> | |
| <Tabs.Item.Icon icon={configureIcon} /> | |
| <Tabs.Item.Text>Icon</Tabs.Item.Text> | |
| </Tabs.Item> | |
| ) : ( | |
| <Tabs.Item {...props}>Tab</Tabs.Item> | |
| )} | |
| </Tabs.List> | |
| </Tabs> | |
| )} | |
| </ComponentStatesTable> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/preview-react/tabs/stories/testing.stories.tsx` around lines 52 - 64,
Wrap the conditional Tabs.Item instances in a Tabs.List within the Tabs
component in the ComponentStatesTable render callback. Preserve the existing
hasIcon, variant, and item content behavior while ensuring both branches are
children of Tabs.List.
| }, | ||
| }, | ||
|
|
||
| '&[aria-selected=true]': { |
There was a problem hiding this comment.
Can we add a non-color selected indicator for the filled variant that survives Windows High Contrast / forced-colors themes? Right now the selected state appears to rely on background color only.
(Regression from our Tabs in main because they use a bottom-border on selected state.)
This is a red flag for me. The color difference between selected and unselected states does not meet contrast guidelines and much too difficult to see. CC: @purvas12
There was a problem hiding this comment.
Thanks for the callout @williamjstanton - left a question for design on this. I assume that the border that shows on selected tabs is enough of a differentiation for the outlined variant? If so, then would adding a border that's visible on selected tabs for the default (filled) variant when we detect high contrast mode (via '@media (forced-colors: active)' which we use elsewhere) be enough of a visual indication?
| )} | ||
| > | ||
| {useListRenderItems(model, children)} | ||
| {overflowButton} |
There was a problem hiding this comment.
Since this renders a non-tab menu button directly inside role="tablist", can we confirm the ARIA pattern here or document the exception? Existing tests suppress aria-required-children for this structure.”
The changes that I'd really like to see are:
- Include this overflow button into the collection system so the roving tabindex right / left arrow key interaction can work on this "More" tab,
- Set required
role="tab" - Set
aria-haspopup="menu"
I worked on these changes here in my branch:
williamjstanton/canvas-kit/tree/william-3558-overflow-tabs-fixes
Issue: #3558
|
|
||
| ### Named Tabs | ||
|
|
||
| `Tabs.Item` and `Tabs.Panel` both take an optional `data-id` attribute that is used for the |
There was a problem hiding this comment.
Should this say onSelect instead of onActivate to match the model API?
| */ | ||
| 'aria-controls'?: string; | ||
| /** | ||
| * Part of the ARIA specification for tabs. Lets screen readers know which tab is active. This |
There was a problem hiding this comment.
This says aria-selected should never be false, but useTabsItem returns false for inactive tabs. I think the implementation is correct according to W3C spec. We just need to update this comment and any related docs.
There was a problem hiding this comment.
Thanks William! That sounds right to me -- Removed that part of the comment.
Summary
Fixes: #3980
New Tabs component has been added to preview.
Release Category
Components
Release Note
New Tabs component has been added to preview.
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)
Summary by CodeRabbit