feat (1-breadcrumb): add automatic ellipsis support and hide items#667
Open
paanSinghCoder wants to merge 1 commit intomainfrom
Open
feat (1-breadcrumb): add automatic ellipsis support and hide items#667paanSinghCoder wants to merge 1 commit intomainfrom
paanSinghCoder wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThe changes introduce an auto-collapse feature to the Breadcrumb component with new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Suggested labels
Suggested reviewers
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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/raystack/components/breadcrumb/breadcrumb-root.tsx (3)
148-166:keyedhelper redundantly checksisValidElement.The
keyedfunction is only ever called with elements frombeforeItemsandafterItems, which are already validated asReactElementinstances fromparseBreadcrumbChildren. TheisValidElement(el)check is unnecessary.♻️ Optional simplification
- const keyed = (el: ReactElement, k: string) => - isValidElement(el) && el.key != null - ? el - : cloneElement(el, { key: k }); + const keyed = (el: ReactElement, k: string) => + el.key != null ? el : cloneElement(el, { key: k });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 148 - 166, The keyed helper unnecessarily calls isValidElement; since beforeItems/afterItems are already ReactElement instances from parseBreadcrumbChildren, remove the isValidElement check in the keyed function (breadcrumb-root.tsx) and simply return the element if el.key != null, otherwise return cloneElement(el, { key: k }); Update the keyed helper used where beforeItems/afterItems are iterated so it only checks el.key and clones when missing, keeping cloneElement usage and keys (e.g., calls in the beforeItems.forEach and afterItems.forEach loops) intact.
69-88: Consider handling unrecognized children gracefully.The
parseBreadcrumbChildrenfunction silently drops children that don't match known breadcrumb parts (Item, Separator, Ellipsis). This could lead to confusion if users pass custom elements or text nodes. Consider either logging a warning in development or documenting this behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 69 - 88, parseBreadcrumbChildren currently drops children that aren't recognized breadcrumb parts (BreadcrumbItem, BreadcrumbSeparator, BreadcrumbEllipsis) with no feedback; update parseBreadcrumbChildren to warn in development when an element is a valid React element but not matched by isBreadcrumbPart (and optionally when a non-empty text node or unknown node is passed) by calling console.warn or process.env.NODE_ENV check inside the flat.forEach loop; reference the existing symbols parseBreadcrumbChildren, flattenFragments(Children.toArray(children)), isValidElement, isBreadcrumbPart, BreadcrumbItem, BreadcrumbSeparator, and BreadcrumbEllipsis and emit a clear dev-only warning that includes the offending child/type so users know why their node was dropped.
171-173: Unused_propsChildrenextraction appears defensive but unnecessary.The destructuring on line 171-173 extracts
childrenfrompropsto avoid passing it twice to thenav. However,childrenwas already destructured at line 116, so it shouldn't be inprops. This suggests the typing could be tightened, but it's harmless as written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx` around lines 171 - 173, The extraction of children into _propsChildren from props (const { children: _propsChildren, ...restProps } = props) is redundant because children is already destructured earlier; remove that destructuring and ensure you use the existing children variable and a restProps that does not re-extract children (or tighten the prop typing to exclude children from props) so children is not handled twice in BreadcrumbRoot; update references to use restProps (or a propsWithoutChildren alias) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/app/examples/page.tsx`:
- Around line 61-66: The Breadcrumb.Item for the "Footwear" label has the wrong
href; update the href on the Breadcrumb.Item (the JSX element with label
"Footwear") to the correct path (e.g. change
'/products/electronics/laptops/gaming/accessories' to
'/products/electronics/laptops/gaming/accessories/footwear') so the link matches
the label.
---
Nitpick comments:
In `@packages/raystack/components/breadcrumb/breadcrumb-root.tsx`:
- Around line 148-166: The keyed helper unnecessarily calls isValidElement;
since beforeItems/afterItems are already ReactElement instances from
parseBreadcrumbChildren, remove the isValidElement check in the keyed function
(breadcrumb-root.tsx) and simply return the element if el.key != null, otherwise
return cloneElement(el, { key: k }); Update the keyed helper used where
beforeItems/afterItems are iterated so it only checks el.key and clones when
missing, keeping cloneElement usage and keys (e.g., calls in the
beforeItems.forEach and afterItems.forEach loops) intact.
- Around line 69-88: parseBreadcrumbChildren currently drops children that
aren't recognized breadcrumb parts (BreadcrumbItem, BreadcrumbSeparator,
BreadcrumbEllipsis) with no feedback; update parseBreadcrumbChildren to warn in
development when an element is a valid React element but not matched by
isBreadcrumbPart (and optionally when a non-empty text node or unknown node is
passed) by calling console.warn or process.env.NODE_ENV check inside the
flat.forEach loop; reference the existing symbols parseBreadcrumbChildren,
flattenFragments(Children.toArray(children)), isValidElement, isBreadcrumbPart,
BreadcrumbItem, BreadcrumbSeparator, and BreadcrumbEllipsis and emit a clear
dev-only warning that includes the offending child/type so users know why their
node was dropped.
- Around line 171-173: The extraction of children into _propsChildren from props
(const { children: _propsChildren, ...restProps } = props) is redundant because
children is already destructured earlier; remove that destructuring and ensure
you use the existing children variable and a restProps that does not re-extract
children (or tighten the prop typing to exclude children from props) so children
is not handled twice in BreadcrumbRoot; update references to use restProps (or a
propsWithoutChildren alias) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65b1ecac-7a92-4967-8461-a9e16b6675b4
📒 Files selected for processing (7)
apps/www/src/app/examples/page.tsxapps/www/src/components/demo/demo.tsxapps/www/src/content/docs/components/breadcrumb/demo.tsapps/www/src/content/docs/components/breadcrumb/index.mdxapps/www/src/content/docs/components/breadcrumb/props.tspackages/raystack/components/breadcrumb/__tests__/breadcrumb.test.tsxpackages/raystack/components/breadcrumb/breadcrumb-root.tsx
Comment on lines
+61
to
+66
| <Breadcrumb.Item | ||
| href='/products/electronics/laptops/gaming/accessories' | ||
| current | ||
| > | ||
| Footwear | ||
| </Breadcrumb.Item> |
Contributor
There was a problem hiding this comment.
Inconsistent href for "Footwear" item.
The href for "Footwear" is /products/electronics/laptops/gaming/accessories but the label is "Footwear". This appears to be a copy-paste error—the href should likely be /products/electronics/laptops/gaming/accessories/footwear or similar.
🐛 Suggested fix
<Breadcrumb.Item
- href='/products/electronics/laptops/gaming/accessories'
+ href='/products/electronics/laptops/gaming/accessories/footwear'
current
>
Footwear
</Breadcrumb.Item>📝 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
| <Breadcrumb.Item | |
| href='/products/electronics/laptops/gaming/accessories' | |
| current | |
| > | |
| Footwear | |
| </Breadcrumb.Item> | |
| <Breadcrumb.Item | |
| href='/products/electronics/laptops/gaming/accessories/footwear' | |
| current | |
| > | |
| Footwear | |
| </Breadcrumb.Item> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/www/src/app/examples/page.tsx` around lines 61 - 66, The Breadcrumb.Item
for the "Footwear" label has the wrong href; update the href on the
Breadcrumb.Item (the JSX element with label "Footwear") to the correct path
(e.g. change '/products/electronics/laptops/gaming/accessories' to
'/products/electronics/laptops/gaming/accessories/footwear') so the link matches
the label.
paanSinghCoder
changed the title
paanSinghCoder
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Breadcrumb 1st PR
Issue-600
Description
New props (root)
maxItems– Maximum number of breadcrumb items to show. When there are more items, the trail collapses: a fixed number at the start, ellipsis, then the rest at the end. At least 2 items are always visible (1 before and 1 after the ellipsis). Values < 2 are treated as 2.itemsBeforeCollapse– Number of items to show before the ellipsis when collapsed. If not set, it is derived frommaxItems(e.g.maxItems={5}→ 2 before, 3 after). The count after the ellipsis is always derived.Docs updates
maxItems, anditemsBeforeCollapse.maxItems, anditemsBeforeCollapsewith more items to show collapse.className, note oncurrentprop, size example with tabs).Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
New Features
Documentation