feat(header): ✨ add navigation menu with dropdown support - #814
aXenDeveloper wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0473ff85ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| discover: t(HEADER_NAV_MESSAGE_KEYS.discover), | ||
| search: t(HEADER_NAV_MESSAGE_KEYS.search), | ||
| }), | ||
| ...EXAMPLE_NAV_ITEMS, |
There was a problem hiding this comment.
Every consumer of MainHeader, including the fresh create-vitnode-app scaffold, now receives these demo Community/Resources entries unconditionally. That scaffold has no plugins and its registered core routes include /discover and /search but not /docs, so selecting Resources → Documentation reaches the not-found page; the hard-coded English labels also bypass the active locale. Remove the examples from the library default or accept real application-provided navigation instead.
Useful? React with 👍 / 👎.
| {...props} | ||
| /> | ||
| > | ||
| <span |
There was a problem hiding this comment.
NavigationMenuPrimitive.List renders an HTMLUListElement, but this adds a <span> as its direct child alongside the menu's <li> items. A <ul> may only contain list items and script-supporting elements, so this produces invalid list markup and can interfere with assistive technology's list representation; render the decoration outside the list or through CSS instead.
AGENTS.md reference: AGENTS.md:L56-L56
Useful? React with 👍 / 👎.
| onFocus={event => { | ||
| const item = itemUnder(event.target); | ||
| if (item) moveHighlightTo(item); | ||
| onFocus?.(event); | ||
| }} |
There was a problem hiding this comment.
When a keyboard user focuses an inactive menu item and then tabs completely out of the navigation, this handler makes the highlight visible but no blur handler calls settleHighlight or hides it. With no open trigger or active link mutation to wake the observers, the last focused item remains visibly highlighted after focus has moved elsewhere; handle focus leaving the list analogously to onPointerLeave.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2c0fa327
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return ( | ||
| <NavigationMenuItem> | ||
| <NavigationMenuTrigger className="text-muted-foreground hover:text-foreground data-popup-open:text-foreground"> |
There was a problem hiding this comment.
When the current route matches a dropdown child, only the link inside the closed popup receives active; the visible trigger has styling only for hover and data-popup-open. As a result, users on /docs or another child route get no persistent indication of their current top-level section until they open the menu. Derive the trigger's active state from its children and expose a corresponding active style.
Useful? React with 👍 / 👎.
| > | ||
| <span className="font-medium">{child.label}</span> | ||
| {child.description ? ( | ||
| <span className="text-muted-foreground text-xs leading-relaxed text-pretty"> |
There was a problem hiding this comment.
Every dropdown description is rendered with Tailwind's text-xs (12px), below the repository's explicit 14px minimum for body text. This makes the supporting navigation copy unnecessarily difficult to read; use text-sm or another size of at least 14px.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
| /** One entry in the main nav - a link, or a dropdown when it carries `items`. */ | ||
| export interface HeaderNavItem { | ||
| href: string; | ||
| items?: HeaderNavChildItem[]; |
There was a problem hiding this comment.
This adds the items contract and dropdown behavior without documenting how navigation entries, child descriptions, and active routes are intended to be configured; a repo-wide documentation search found no HeaderNavItem or HeaderNavMenu guidance. Add concise documentation for the feature as required by the repository contribution rules.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
1 participant
Improving Documentation
pnpm lint:fixto fix formatting issues before opening the PR.Description
What?
Why?