feat: add Breadcrumb component#21
Conversation
Accepts a variable-length array of items with label, optional href or onClick. Supports custom link rendering via renderLink prop for framework-specific routing (e.g. Next.js Link). Uses semantic HTML with nav > ol > li, aria-current="page" on the last item, and ChevronRightIcon separators. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughI pity the fool who misses this: adds a client-side React Breadcrumb component, tests, Storybook stories, and barrel exports — renders Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ui/Breadcrumb/Breadcrumb.test.tsx (1)
93-128: Props and many items tested right - but Mr. T notices somethin' missing!These tests are good, fool! But Mr. T doesn't see a test for an empty
itemsarray. Might wanna add one to make sure the component handles that edge case gracefully.🧪 Suggested test for empty items
it("renders nothing for empty items array", () => { render(<Breadcrumb items={[]} />); const list = document.querySelector('[data-slot="breadcrumb-list"]'); expect(list?.querySelectorAll("li")).toHaveLength(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Breadcrumb/Breadcrumb.test.tsx` around lines 93 - 128, Add a new test in Breadcrumb.test.tsx that verifies Breadcrumb handles an empty items array: render <Breadcrumb items={[]} />, select the element with data-slot="breadcrumb-list" (or the list slot used by the component) and assert it contains zero <li> children; name the test something like "renders nothing for empty items array" and place it alongside the other it(...) blocks so the Breadcrumb component's empty-array edge case is covered.src/components/ui/Breadcrumb/Breadcrumb.tsx (1)
55-74: Listen here, fool! Mr. T sees an inconsistency!When using the native
<a>tag, you renderitem.labeldirectly and applydata-slot="breadcrumb-link"to the anchor. But when usingrenderLink, you pass a<span>wrapper as children and the custom link element won't have thedata-slotattribute.This creates inconsistent markup between the two code paths, which could affect styling hooks and testing.
♻️ Proposed fix to align both code paths
if (renderLink) { - return renderLink(item.href, linkContent); + return ( + <span data-slot="breadcrumb-link"> + {renderLink(item.href, linkContent)} + </span> + ); }Or alternatively, pass the label directly to
renderLinkfor consistency:- const linkContent = ( - <span className="text-gray-500 hover:text-gray-700"> - {item.label} - </span> - ); - if (renderLink) { - return renderLink(item.href, linkContent); + return renderLink(item.href, item.label); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Breadcrumb/Breadcrumb.tsx` around lines 55 - 74, The breadcrumb renders inconsistent markup: the native <a> uses data-slot="breadcrumb-link" and renders item.label directly, while the renderLink path wraps the label in a <span> without the data-slot; update the Breadcrumb component so both paths produce the same markup by either passing the label (item.label) directly to renderLink or passing the same anchor-like children with data-slot="breadcrumb-link" to renderLink; locate this logic in the render block that checks if (!isLast && item.href) and adjust the call to renderLink(item.href, ...) to include the data-slot attribute (or plain label) so styling/testing hooks match the native <a> output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/ui/Breadcrumb/Breadcrumb.tsx`:
- Around line 88-93: The current JSX in Breadcrumb (the map rendering each
BreadcrumbItem using key={item.label}) can produce duplicate keys; update the
rendering in Breadcrumb.tsx to use a stable unique key instead — either use the
map index (e.g., key={index}) if items are static, or add and use an explicit id
property on the BreadcrumbItem type (e.g., item.id) and set key={item.id};
update the BreadcrumbItem type/interface if you choose id so callers supply it,
and replace key={item.label} in the component accordingly.
---
Nitpick comments:
In `@src/components/ui/Breadcrumb/Breadcrumb.test.tsx`:
- Around line 93-128: Add a new test in Breadcrumb.test.tsx that verifies
Breadcrumb handles an empty items array: render <Breadcrumb items={[]} />,
select the element with data-slot="breadcrumb-list" (or the list slot used by
the component) and assert it contains zero <li> children; name the test
something like "renders nothing for empty items array" and place it alongside
the other it(...) blocks so the Breadcrumb component's empty-array edge case is
covered.
In `@src/components/ui/Breadcrumb/Breadcrumb.tsx`:
- Around line 55-74: The breadcrumb renders inconsistent markup: the native <a>
uses data-slot="breadcrumb-link" and renders item.label directly, while the
renderLink path wraps the label in a <span> without the data-slot; update the
Breadcrumb component so both paths produce the same markup by either passing the
label (item.label) directly to renderLink or passing the same anchor-like
children with data-slot="breadcrumb-link" to renderLink; locate this logic in
the render block that checks if (!isLast && item.href) and adjust the call to
renderLink(item.href, ...) to include the data-slot attribute (or plain label)
so styling/testing hooks match the native <a> output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8cfa0f61-0c8a-4885-a49a-2877f15c82c7
📒 Files selected for processing (4)
src/components/ui/Breadcrumb/Breadcrumb.test.tsxsrc/components/ui/Breadcrumb/Breadcrumb.tsxsrc/components/ui/Breadcrumb/index.tssrc/components/ui/index.ts
…sues Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sort JSX props in SignContractIcon and fix export order in icons/index.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Breadcrumbcomponent accepting a variable-lengthitemsarray withlabel, optionalhreforonClickrenderLinkprop (e.g. Next.jsLink)nav > ol > li),aria-current="page"on last item,ChevronRightIconseparatorsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Style