fix(spx-gui): tooltip side-placement alignment bug#3057
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the default offset logic for floating popups, specifically for the UITooltip component. It replaces a hardcoded vertical offset with a dynamic resolveDefaultPopupOffset function that calculates the appropriate main-axis gap based on the popup's placement and arrow visibility. This ensures consistent spacing for both horizontal and vertical placements. The changes include new unit tests for the offset resolution logic and updated tests for UITooltip and useFloatingPopup. I have no feedback to provide as there are no review comments.
There was a problem hiding this comment.
The fix is correct and well-structured. Centralizing the arrow-aware gap in resolveDefaultPopupOffset cleanly eliminates the duplicated magic number and properly addresses the misalignment for side placements. No security or performance concerns.
A few items worth addressing:
There was a problem hiding this comment.
Pull request overview
Fixes tooltip misalignment for side placements by moving arrow-gap handling into the shared useFloatingPopup positioning logic, ensuring consistent main-axis spacing across placements when arrows are enabled.
Changes:
- Derive the default popup offset from
placement+POPUP_ARROW_SIZEwhenshowArrowis true (resolveDefaultPopupOffset). - Remove tooltip’s hard-coded
{ x: 0, y: 8 }offset so right/left placements no longer get an incorrect vertical shift. - Add/extend unit tests to validate default offset behavior and middleware configuration for tooltip placements.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| spx-gui/src/components/ui/popup/use-floating-popup.ts | Introduces placement-aware default offset for arrow-enabled popups via resolveDefaultPopupOffset. |
| spx-gui/src/components/ui/popup/use-floating-popup.test.ts | Adds tests for the new default offset behavior and asserts offset middleware configuration. |
| spx-gui/src/components/ui/UITooltip.vue | Removes tooltip-specific offset so spacing is handled centrally by useFloatingPopup. |
| spx-gui/src/components/ui/UITooltip.test.ts | Adds coverage ensuring right/top-end placements produce main-axis offset without unintended cross-axis shift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix tooltip side-placement offset handling and centralize arrow-based default popup gaps in
useFloatingPopup.Previously
UITooltipalways passed a vertical{ x: 0, y: 8 }gap, which is only correct for top/bottom placements; forrightit incorrectly shifted the popup downward.The default gap is now derived from placement and
POPUP_ARROW_SIZEinsideuseFloatingPopup, so arrow-enabled popups keep a consistent gap without duplicating magic numbers in individual consumers.