-
-
Notifications
You must be signed in to change notification settings - Fork 84
Add pointer cursor when hover over breadcrumbs #608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
WalkthroughAdded Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
view/components/layout/dashboard-layout.tsx (2)
86-91: Remove redundantcursor-pointerclass.The
BreadcrumbLinkcomponent already appliescursor-pointerin its base implementation (seeview/components/ui/breadcrumb.tsxline 46), making this explicit class redundant.Apply this diff to remove the duplication:
<BreadcrumbLink onClick={() => router.push(breadcrumb.href)} - className="cursor-pointer" > {breadcrumb.label} </BreadcrumbLink>
86-91: Consider addinghrefattribute for better accessibility.The breadcrumb links use
onClickfor navigation but lack anhrefattribute. Addinghrefwould improve accessibility (keyboard navigation, screen readers), enable right-click context menu options, and provide better SEO crawlability.You could apply this diff if using native anchor behavior is acceptable:
<BreadcrumbLink + href={breadcrumb.href} onClick={() => router.push(breadcrumb.href)} > {breadcrumb.label} </BreadcrumbLink>Alternatively, consider using Next.js
Linkcomponent with theasChildprop for better SPA integration:import Link from 'next/link'; <BreadcrumbLink asChild> <Link href={breadcrumb.href}> {breadcrumb.label} </Link> </BreadcrumbLink>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
view/components/layout/dashboard-layout.tsx(1 hunks)view/components/ui/breadcrumb.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
view/components/layout/dashboard-layout.tsx (1)
view/components/ui/breadcrumb.tsx (1)
BreadcrumbLink(98-98)
view/components/ui/breadcrumb.tsx (1)
view/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (1)
view/components/ui/breadcrumb.tsx (1)
46-46: LGTM! Good UX improvement.Adding
cursor-pointerprovides consistent visual feedback across different use cases, especially when using theasChildprop with components that aren't native anchor tags.
Description
This PR adds an improved mouse-over interaction for breadcrumb items. When hovering over a breadcrumb, it now provides a clearer visual indication
Scope of Change
Select all applicable areas impacted by this PR:
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.