Fix: Bold text on Community dropdown #2076
Conversation
Summary by CodeRabbit
WalkthroughAdjusted inactive submenu item styling to include Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(none) Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/components/NavDropDown.tsx (1)
82-89: Don’t prevent default on Enter for submenu links; it can block navigationPreventing default on Enter for can cancel the implicit click in some browsers, harming keyboard navigation. Let Enter pass through; optionally support Space by translating it into a click.
Apply this diff:
- onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - e.preventDefault() - setIsOpen(false) - } else if (e.key === 'Escape') { - setIsOpen(false) - } - }} + onKeyDown={(e) => { + if (e.key === ' ') { + // Support spacebar activation without breaking navigation + e.preventDefault() + e.currentTarget.click() + } else if (e.key === 'Escape') { + e.preventDefault() + setIsOpen(false) + } + }}
🧹 Nitpick comments (5)
frontend/src/components/NavDropDown.tsx (5)
79-79: Confirm weight choice: consider font-normal if “regular” weight is desiredIf the intended look for inactive items is the default/regular weight (not medium), switch to font-normal to fully match prior styling on non-Community pages.
Apply if desired:
- : 'font-medium text-slate-600 hover:bg-slate-100 hover:text-slate-900 dark:text-slate-400 dark:hover:bg-slate-700 dark:hover:text-white' + : 'font-normal text-slate-600 hover:bg-slate-100 hover:text-slate-900 dark:text-slate-400 dark:hover:bg-slate-700 dark:hover:text-white'
38-40: Prefer some() over map().includes() to avoid extra allocations and clarify intentThis avoids creating an intermediate array of hrefs each render and reads closer to the intent.
Apply this diff:
- link.submenu.map((sub) => sub.href).includes(pathname) && + link.submenu.some((sub) => sub.href === pathname) &&
15-15: Type the ref for safer DOM usageTyping the ref prevents TS errors around .contains and improves editor IntelliSense.
Apply this diff:
- const dropdownRef = useRef(null) + const dropdownRef = useRef<HTMLDivElement>(null)
20-24: Guard event.target with a Node check to satisfy TS and edge casesEventTarget isn’t guaranteed to be a Node. Add a type guard before calling .contains.
Apply this diff:
- function handleClickOutside(event: MouseEvent) { - if (dropdownRef.current && !dropdownRef.current.contains(event.target)) { + function handleClickOutside(event: MouseEvent) { + if ( + dropdownRef.current && + event.target instanceof Node && + !dropdownRef.current.contains(event.target) + ) { setIsOpen(false) } }
73-73: Avoid using index as React key; prefer a stable identifierUsing href (or another unique field) avoids unnecessary re-mounts when the list changes.
Apply this diff:
- key={idx} + key={submenu.href ?? String(idx)}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/src/components/NavDropDown.tsx(1 hunks)
🔇 Additional comments (1)
frontend/src/components/NavDropDown.tsx (1)
79-79: Fix correctly prevents inherited bold in submenu itemsAdding font-medium on the inactive branch overrides the parent’s font-bold when on a submenu route. This directly addresses the reported issue.
Srijatalamarla
changed the title
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
This looks good! 👍🏼
Could you also add a couple of tests to check for styling in active/inactive state? Nothing crazy, just these two cases.
|
Sure! I'll add the tests soon. Thanks for the review! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/NavDropDown.test.tsx (2)
132-148: Solid targeted test for active submenu styling; consider making it more robustTwo small improvements:
- Use findByRole after opening the menu to avoid timing flakiness.
- Add negative assertions to ensure inactive-only classes aren’t present on the active item.
Apply this diff:
it('applies active styling to submenu item when pathname matches', async () => { const propsWithActiveSubmenu = { pathname: '/docs/getting-started', link: mockLink, } const user = userEvent.setup() render(<NavDropdown {...propsWithActiveSubmenu} />) const button = screen.getByRole('button') await user.click(button) //active - const gettingStartedLink = screen.getByRole('link', { name: /getting started/i }) - - expect(gettingStartedLink).toHaveClass('font-bold', 'text-white') + const gettingStartedLink = await screen.findByRole('link', { name: /getting started/i }) + expect(gettingStartedLink).toHaveClass('font-bold', 'text-white') + expect(gettingStartedLink).not.toHaveClass('font-medium', 'text-slate-600') })
150-168: Great verification of sibling items; add negative assertions and await DOM updates
- Use findByRole for submenu links post-toggle.
- Assert that active-only classes are not applied to inactive items.
Apply this diff:
it('does not apply active styling to other submenu items when one is active', async () => { const propsWithActiveSubmenu = { pathname: '/docs/getting-started', link: mockLink, } const user = userEvent.setup() render(<NavDropdown {...propsWithActiveSubmenu} />) const button = screen.getByRole('button') await user.click(button) //inactive - const apiReferenceLink = screen.getByRole('link', { name: /api reference/i }) - const examplesLink = screen.getByRole('link', { name: /examples/i }) - - expect(apiReferenceLink).toHaveClass('font-medium', 'text-slate-600') - expect(examplesLink).toHaveClass('font-medium', 'text-slate-600') + const apiReferenceLink = await screen.findByRole('link', { name: /api reference/i }) + const examplesLink = await screen.findByRole('link', { name: /examples/i }) + + expect(apiReferenceLink).toHaveClass('font-medium', 'text-slate-600') + expect(examplesLink).toHaveClass('font-medium', 'text-slate-600') + expect(apiReferenceLink).not.toHaveClass('font-bold', 'text-white') + expect(examplesLink).not.toHaveClass('font-bold', 'text-white') })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
frontend/__tests__/unit/components/NavDropDown.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/__tests__/unit/components/NavDropDown.test.tsx (2)
frontend/src/wrappers/testUtil.tsx (1)
render(14-14)frontend/src/components/NavDropDown.tsx (1)
NavDropdown(13-98)
🔇 Additional comments (1)
frontend/__tests__/unit/components/NavDropDown.test.tsx (1)
132-168: These tests capture the original bug and the intended fix preciselyThey validate that only the active submenu item is bolded and siblings remain medium-weight, matching the updated class logic. Nice addition to prevent regression.
|
2 participants
Proposed change
Resolves #2035
Before fix:
After fix:
Checklist
make check-testlocally; all checks and tests passed.