feat(NavigationMenu): allow tooltip usage in horizontal orientation #5682
feat(NavigationMenu): allow tooltip usage in horizontal orientation #5682benjamincanac merged 15 commits intonuxt:v4from
horizontal orientation #5682Conversation
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional Suggestion:
The documentation comments describe tooltip behavior as "when the menu is collapsed," but the code now enables tooltips in horizontal orientation (where the collapsed state doesn't apply). The documentation should be updated to reflect that tooltips can now show in both vertical+collapsed and horizontal orientations.
View Details
📝 Patch Details
diff --git a/src/runtime/components/NavigationMenu.vue b/src/runtime/components/NavigationMenu.vue
index 322fc481..72fd97d3 100644
--- a/src/runtime/components/NavigationMenu.vue
+++ b/src/runtime/components/NavigationMenu.vue
@@ -28,7 +28,7 @@ export interface NavigationMenuItem extends Omit<LinkProps, 'type' | 'raw' | 'cu
*/
badge?: string | number | BadgeProps
/**
- * Display a tooltip on the item when the menu is collapsed with the label of the item.
+ * Display a tooltip on the item when the menu is collapsed (vertical orientation) or in horizontal orientation with the label of the item.
* This has priority over the global `tooltip` prop.
*/
tooltip?: boolean | TooltipProps
@@ -140,7 +140,7 @@ export interface NavigationMenuProps<
*/
collapsed?: boolean
/**
- * Display a tooltip on the items when the menu is collapsed with the label of the item.
+ * Display a tooltip on the items when the menu is collapsed in vertical orientation or in horizontal orientation with the label of the item.
* `{ delayDuration: 0, content: { side: 'right' } }`{lang="ts-type"}
* @defaultValue false
*/
Analysis
Inaccurate tooltip documentation in NavigationMenu component
What fails: Documentation comments for the tooltip prop in NavigationMenu.vue (lines 30-31 and 143-144) state "Display a tooltip on the item/items when the menu is collapsed" but don't account for horizontal orientation behavior.
How to reproduce: Examine the code at line 402 where the tooltip condition is:
v-else-if="(((orientation === 'vertical' && collapsed) || orientation === 'horizontal') && (!!props.tooltip || !!item.tooltip))"
And compare with the JSDoc comments at lines 30-31 and 143-144 which only mention "when the menu is collapsed".
Result: The documentation misleads users about when tooltips display. The comments suggest tooltips only show when collapsed=true, but they also display in horizontal orientation where the collapsed prop doesn't apply (as documented at lines 138-140: "Only works when orientation is vertical").
Expected: Documentation should explicitly state that tooltips display in:
- Vertical orientation when the menu is collapsed (only then, since
collapsedonly works in vertical mode) - Horizontal orientation (always, when tooltip prop is enabled)
Reference: Commit 7551c3d ("fix(navigationmenu): make possible to use tooltip in horizontal orientation") intentionally changed the tooltip condition to support horizontal orientation but did not update the corresponding documentation comments.
|
I just read the comment from the Vercel bot, and he’s absolutely right. If you’re open to accepting this change, Benjamin, would you prefer a different approach? If so, could you please provide me some information so I can propose a compromise that you’d find acceptable? |
commit: |
|
I think it is correct as well as I don't think we should simply enable tooltips in horizontal orientation, it will conflict with the actual menu. We should rather handle |
|
Would this approach be more effective to you? |
|
Hey Benjamin, I hope you enjoyed the holiday season. I'm coming back to this PR to see if it suited you and if you could therefore possibly accept it. Happy new year ! |
benjamincanac
changed the title
horizontal orientation
benjamincanac
left a comment
benjamincanac
left a comment
There was a problem hiding this comment.
@EvanSchleret Happy new year as well! 😊
I'm not sure to understand your changes, the hidden class is already applied in src/theme/navigation-menu.ts when collapsed is true. I think we can keep the same behavior in vertical and horizontal orientations, there's no need for a tooltip if the label is displayed anyway.
remove component-level horizontal label/trailing hiding logic and rely on navigation-menu theme classes for collapsed behavior restore docs wording for collapsed + tooltip to match vertical orientation semantics
|
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:
📝 WalkthroughWalkthroughTooltip behavior in NavigationMenu.vue was adjusted: tooltips now appear for items with an explicit Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/components/NavigationMenu.vue (1)
136-141:⚠️ Potential issue | 🟡 MinorUpdate the
collapsedprop JSDoc to reflect horizontal orientation support.The docstring says
collapsedonly works whenorientationisvertical, but the template now allows tooltip rendering whencollapsedis true regardless of orientation. If this is intentional, the documentation should be updated./** * Collapse the navigation menu to only show icons. - * Only works when `orientation` is `vertical`. * `@defaultValue` false */ collapsed?: boolean
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/components/NavigationMenu.vue (1)
403-407:⚠️ Potential issue | 🟠 MajorTooltip is still gated behind
orientation === 'vertical' && collapsed— PR objective not met.The stated goal is to allow tooltip usage in horizontal orientation, but the
v-else-ifcondition on Line 403 still requiresorientation === 'vertical'andcollapsed. The only change here is a trailing whitespace in the tag. No behavioral change is introduced by this PR.Given the maintainer's feedback about handling
collapsedbehavior in horizontal orientation, the condition and surrounding logic need to be updated to actually support the new use case.
🤖 Fix all issues with AI agents
In `@src/runtime/components/NavigationMenu.vue`:
- Line 262: The helper function hasTooltip (signature: const hasTooltip = (item:
NavigationMenuItem): boolean => !!props.tooltip || !!item.tooltip) is dead code;
either wire it into the template by replacing the inline check (!!props.tooltip
|| !!item.tooltip) with hasTooltip(item) and expose hasTooltip from the
setup/return so the template can call it, or remove the hasTooltip declaration
entirely to eliminate dead code—update the template references and module
exports accordingly.
|
Hey @benjamincanac, I hope you’re doing well. I completely forgot about my PR and your last comment. I made some changes based on your feedback. Would this be a better solution for you? Thanks for your answer! |
…mode (Sorry, I committed the wrong conflict resolution previously ...) this commit restores the intended fix: - in horizontal + collapsed mode, only tooltip-enabled items collapse to icon-only - non-tooltip items keep their label/trailing visible - tooltip rendering in horizontal mode is enabled only for tooltip-enabled items
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/content/docs/2.components/navigation-menu.md (2)
337-337: Consider clarifying "tooltip-enabled items".The phrase "tooltip-enabled items" is understandable, but could be more explicit. Consider rephrasing to "items that have a tooltip configured" or "items with the
tooltipproperty set" for additional clarity.✍️ Suggested wording improvement
-When the menu is `collapsed`, in `vertical` orientation labels and trailing content are hidden for all items. In `horizontal` orientation, only tooltip-enabled items are collapsed to icon-only. +When the menu is `collapsed`, in `vertical` orientation labels and trailing content are hidden for all items. In `horizontal` orientation, only items with a `tooltip` configured are collapsed to icon-only.
985-985: Clarify global vs. per-item tooltip behavior in horizontal orientation.The documentation explains tooltip usage in both orientations, but it could be more explicit about whether the global
tooltipprop applies in horizontal orientation or if only per-itemtooltipproperties work. Consider adding a clarification.✍️ Suggested wording improvement
-When orientation is `vertical` and the menu is `collapsed`, you can set the `tooltip` prop to `true` to display a [Tooltip](/docs/components/tooltip) around items with their label. In `horizontal` orientation, use `tooltip` on each item to collapse and display a tooltip only on those specific items. +When orientation is `vertical` and the menu is `collapsed`, you can set the global `tooltip` prop to `true` to display a [Tooltip](/docs/components/tooltip) around all items with their label. In `horizontal` orientation, tooltips must be configured individually on each item using the `tooltip` property to collapse and display a tooltip for those specific items.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/runtime/components/NavigationMenu.vue (2)
428-428: Tooltipside: 'right'default may be suboptimal for horizontal orientation.The shared
tooltipProps(Line 275) defaults to{ content: { side: 'right' } }, which makes sense for vertical collapsed menus. For horizontal menus,side: 'bottom'is typically more appropriate to avoid overlapping adjacent items.Consider adjusting the default side based on orientation, or document that horizontal consumers should override the tooltip side via the per-item
tooltipprop.💡 Possible approach
- const tooltipProps = toRef(() => defu(typeof props.tooltip === 'boolean' ? {} : props.tooltip, { delayDuration: 0, content: { side: 'right' } }) as TooltipProps) + const tooltipProps = toRef(() => defu(typeof props.tooltip === 'boolean' ? {} : props.tooltip, { delayDuration: 0, content: { side: props.orientation === 'vertical' ? 'right' : 'bottom' } }) as TooltipProps)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/NavigationMenu.vue` at line 428, The tooltip default side is hardcoded in tooltipProps to { content: { side: 'right' } }, which is suboptimal for horizontal menus; update the tooltipProps initialization (the variable named tooltipProps used by UTooltip in NavigationMenu.vue) to set content.side conditionally based on the component's orientation (use 'bottom' for orientation === 'horizontal', 'right' otherwise) or compute a resolvedTooltipProps right before the UTooltip v-bind that merges item.tooltip/props.tooltip and overrides the side accordingly so horizontal items get side: 'bottom' while vertical collapsed menus keep side: 'right'.
277-278: Helpers look correct; consider documenting the intentional asymmetry.In vertical mode,
collapsed + props.tooltipaffects all items globally, but in horizontal mode only per-itemitem.tooltiptriggers collapse/tooltip behavior (the globaltooltipprop is ignored). This is intentional per the reviewer discussion, but the divergence could surprise consumers who set:collapsed="true" :tooltip="true"and expect uniform behavior across orientations.A brief code comment here explaining this design choice would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/NavigationMenu.vue` around lines 277 - 278, Add a brief inline comment above the helper functions explaining the intentional asymmetry: that shouldHideLabel uses props.collapsed plus props.orientation and checks item.tooltip only for horizontal mode (via hasHorizontalTooltip) so the global tooltip prop is ignored in horizontal orientation, while in vertical orientation collapsed + props.tooltip applies globally; reference the helpers hasHorizontalTooltip and shouldHideLabel and the relevant props (props.collapsed, props.orientation, item.tooltip) so future maintainers understand this design choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/components/NavigationMenu.vue`:
- Line 428: The tooltip default side is hardcoded in tooltipProps to { content:
{ side: 'right' } }, which is suboptimal for horizontal menus; update the
tooltipProps initialization (the variable named tooltipProps used by UTooltip in
NavigationMenu.vue) to set content.side conditionally based on the component's
orientation (use 'bottom' for orientation === 'horizontal', 'right' otherwise)
or compute a resolvedTooltipProps right before the UTooltip v-bind that merges
item.tooltip/props.tooltip and overrides the side accordingly so horizontal
items get side: 'bottom' while vertical collapsed menus keep side: 'right'.
- Around line 277-278: Add a brief inline comment above the helper functions
explaining the intentional asymmetry: that shouldHideLabel uses props.collapsed
plus props.orientation and checks item.tooltip only for horizontal mode (via
hasHorizontalTooltip) so the global tooltip prop is ignored in horizontal
orientation, while in vertical orientation collapsed + props.tooltip applies
globally; reference the helpers hasHorizontalTooltip and shouldHideLabel and the
relevant props (props.collapsed, props.orientation, item.tooltip) so future
maintainers understand this design choice.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/components/NavigationMenu.vue (1)
425-429:⚠️ Potential issue | 🟡 MinorTooltip + NavigationMenuContent may conflict for items with children in horizontal mode.
When
orientation === 'horizontal'and an item has bothchildrenanditem.tooltipset, theUTooltipwraps the trigger (Line 425), whileNavigationMenuContentis still rendered as a sibling (Line 435 viaNavigationMenuTrigger). Hovering the item could display both the tooltip and the dropdown content panel simultaneously — this is the conflict benjamincanac flagged in the PR discussion.Consider guarding against this by excluding items that have
children(or a content slot) from the horizontal tooltip branch:Suggested fix
- <UTooltip v-else-if="(orientation === 'vertical' && collapsed && (!!props.tooltip || !!item.tooltip)) || (orientation === 'horizontal' && !!item.tooltip)" :text="get(item, props.labelKey as string)" v-bind="{ ...tooltipProps, ...(typeof item.tooltip === 'boolean' ? {} : item.tooltip || {}) }"> + <UTooltip v-else-if="(orientation === 'vertical' && collapsed && (!!props.tooltip || !!item.tooltip)) || (orientation === 'horizontal' && !!item.tooltip && !item.children?.length && !slots[(item.slot ? `${item.slot}-content` : 'item-content') as keyof NavigationMenuSlots<T>])" :text="get(item, props.labelKey as string)" v-bind="{ ...tooltipProps, ...(typeof item.tooltip === 'boolean' ? {} : item.tooltip || {}) }">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/NavigationMenu.vue` around lines 425 - 429, The horizontal-mode tooltip is applied even for items that have dropdown children, causing both UTooltip and NavigationMenuContent to appear; update the UTooltip conditional (the v-else-if around UTooltip) to exclude items with children or a content slot (e.g., add checks like && !item.children && !item.contentSlot / !hasContent) when orientation === 'horizontal', so that items that will render NavigationMenuTrigger/NavigationMenuContent do not get wrapped by UTooltip; adjust any related props/slot checks used by NavigationMenuTrigger to match the same predicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/runtime/components/NavigationMenu.vue`:
- Around line 425-429: The horizontal-mode tooltip is applied even for items
that have dropdown children, causing both UTooltip and NavigationMenuContent to
appear; update the UTooltip conditional (the v-else-if around UTooltip) to
exclude items with children or a content slot (e.g., add checks like &&
!item.children && !item.contentSlot / !hasContent) when orientation ===
'horizontal', so that items that will render
NavigationMenuTrigger/NavigationMenuContent do not get wrapped by UTooltip;
adjust any related props/slot checks used by NavigationMenuTrigger to match the
same predicate.
|
@EvanSchleret I simplified the approach so |
|
Your implementation is actually much better. I’d love to adopt this kind of approach when coding, but I tend to overengineer things, haha! Do you rely "onyl" on your experience, or do you have any advice? If so, I’d like to contribute again to something else here or somewhere else. Thanks! |
|
@EvanSchleret I think it mostly comes down to consistency. With 125+ components in Nuxt UI, the same patterns keep coming back so you naturally end up keeping things simple although the NavigationMenu component is very complicated 😅 I've added a |
|
Thank you! I’ll definitely do it if needed :) |
benjamincanac
changed the title
horizontal orientation horizontal orientation
2 participants
🔗 Linked issue
Resolves #5666
❓ Type of change
📚 Description
With UNavigationMenu, it’s possible to display tooltips, but only in a horizontal orientation. In certain use cases, some users would like to show tooltips for items that don’t have a label for the navigation menu (as seen in the linked issue). This PR addresses this requirement.
📝 Checklist