Improve AppBreadcrumbs and Header components - #570
Conversation
📝 WalkthroughSummary
WalkthroughThe pull request updates Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/oxygen-ui-docs/stories/AppElements/AppBreadcrumbs.stories.tsx`:
- Around line 121-137: Update the JSDoc description for the MiddleItemNoLink
story to accurately state that non-clickable breadcrumb items use disabled text
color, italic styling, and reduced opacity, replacing the incorrect
primary-color and no-hover-effects description.
In `@packages/oxygen-ui/src/components/AppBreadcrumbs/AppBreadcrumbs.tsx`:
- Around line 134-137: Enforce a minimum maxItems value before deriving
visibleItems and hiddenItems in the breadcrumb truncation logic. Normalize
values below 2 to 2, or validate and reject them, so the slice ranges always
preserve the first item and final item while producing valid hiddenItems.
- Around line 139-160: Update renderItem so hidden items without item.onClick
are disabled or otherwise non-interactive, and only attach menu click handling
when item.onClick exists. Preserve the existing clickable behavior for non-last
items with handlers, using the hidden-item rendering path and AppBreadcrumbsItem
props as the implementation points.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a837c225-40b5-459c-a86f-001664ba03ad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.changeset/beige-ducks-glow.mdpackages/oxygen-ui-docs/package.jsonpackages/oxygen-ui-docs/stories/AppElements/AppBreadcrumbs.stories.tsxpackages/oxygen-ui/src/components/AppBreadcrumbs/AppBreadcrumbs.tsxpackages/oxygen-ui/src/components/Header/HeaderToggle.tsxpnpm-workspace.yaml
| /** | ||
| * Demonstrates a breadcrumb with a middle item that has no `onClick` handler. | ||
| * Non-clickable items are rendered in primary text color without hover effects. | ||
| */ | ||
| export const MiddleItemNoLink: Story = { | ||
| render: () => ( | ||
| <AppBreadcrumbs | ||
| items={[ | ||
| {key: 'home', label: 'Home', onClick: () => {}}, | ||
| {key: 'organization', label: 'Organization'}, | ||
| {key: 'project', label: 'Project', onClick: () => {}}, | ||
| {key: 'settings', label: 'Settings'}, | ||
| ]} | ||
| /> | ||
| ), | ||
| }; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the story description.
Line 123 says non-clickable items use primary text color. AppBreadcrumbsItem uses disabled text color, italic styling, and reduced opacity. Update the description to match the rendered behavior.
As per path instructions, provide concise, actionable feedback focused on correctness and best practices.
🤖 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 `@packages/oxygen-ui-docs/stories/AppElements/AppBreadcrumbs.stories.tsx`
around lines 121 - 137, Update the JSDoc description for the MiddleItemNoLink
story to accurately state that non-clickable breadcrumb items use disabled text
color, italic styling, and reduced opacity, replacing the incorrect
primary-color and no-hover-effects description.
Source: Path instructions
| const visibleItems: BreadcrumbItem[] = shouldTruncate | ||
| ? [...items.slice(0, maxItems - 1), items[items.length - 1]] | ||
| : items; | ||
| const hiddenItems: BreadcrumbItem[] = shouldTruncate ? items.slice(maxItems - 1, items.length - 1) : []; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce a minimum maxItems value.
If maxItems is 0 or 1, these slice ranges produce invalid truncation output. Require maxItems >= 2, or normalize lower values before deriving visibleItems and hiddenItems.
As per path instructions, provide concise, actionable feedback focused on correctness and best practices.
🤖 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 `@packages/oxygen-ui/src/components/AppBreadcrumbs/AppBreadcrumbs.tsx` around
lines 134 - 137, Enforce a minimum maxItems value before deriving visibleItems
and hiddenItems in the breadcrumb truncation logic. Normalize values below 2 to
2, or validate and reject them, so the slice ranges always preserve the first
item and final item while producing valid hiddenItems.
Source: Path instructions
| const renderItem = (item: BreadcrumbItem, isLast: boolean) => { | ||
| const isClickable = !isLast && !!item.onClick; | ||
| return ( | ||
| <AppBreadcrumbsItem | ||
| key={item.key} | ||
| variant="h5" | ||
| ownerState={{isLast, isClickable}} | ||
| {...(isClickable && { | ||
| role: 'button', | ||
| tabIndex: 0, | ||
| onClick: item.onClick, | ||
| onKeyDown: (e: React.KeyboardEvent) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| item.onClick?.(); | ||
| } | ||
| } | ||
| > | ||
| {item.label} | ||
| </AppBreadcrumbsItem> | ||
| ); | ||
| }, | ||
| })} | ||
| > | ||
| {item.label} | ||
| </AppBreadcrumbsItem> | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply non-clickable behavior to hidden items.
Line 140 limits the new behavior to visible items. A hidden item without onClick still renders as an enabled MenuItem and can receive activation. Render these entries as disabled or non-interactive, and attach menu click handling only when item.onClick exists.
As per path instructions, provide concise, actionable feedback focused on correctness and best practices.
🤖 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 `@packages/oxygen-ui/src/components/AppBreadcrumbs/AppBreadcrumbs.tsx` around
lines 139 - 160, Update renderItem so hidden items without item.onClick are
disabled or otherwise non-interactive, and only attach menu click handling when
item.onClick exists. Preserve the existing clickable behavior for non-last items
with handlers, using the hidden-item rendering path and AppBreadcrumbsItem props
as the implementation points.
Source: Path instructions
2 participants
This pull request improves the
AppBreadcrumbsandHeadercomponents, adds a new breadcrumb usage example, and updates Storybook and related dependencies to the latest patch version. The changes also include minor adjustments to package references and lock files to ensure consistency.Component improvements:
AppBreadcrumbsto support non-clickable breadcrumb items, which are now styled differently (disabled color, italic, no hover effect) and only render as buttons if anonClickhandler is present. Truncation logic for breadcrumbs was also improved for better UX. [1] [2] [3]HeaderTogglecomponent to use theChevronLefticon instead ofPanelLeftClosefor the collapse action. [1] [2]Documentation and examples:
MiddleItemNoLink) demonstrating a breadcrumb with a non-clickable middle item, clarifying the new visual behavior.Dependency and package updates:
package.jsonandpnpm-lock.yamlfor improved stability and compatibility. [1] [2] [3] [4] [5] [6] [7]lucide-staticand other dependencies to use the correct catalog references for consistency. [1] [2] [3]Release notes: