fix(NavigationMenu): allow tooltip usage in horizontal orientation #5682
fix(NavigationMenu): allow tooltip usage in horizontal orientation #5682EvanSchleret wants to merge 3 commits intonuxt:v4from
horizontal orientation #5682Conversation
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 ! |
horizontal orientation
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.
🔗 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