fix(popover): arrow tracks trigger on collision shift#533
fix(popover): arrow tracks trigger on collision shift#533MaxLee-dev wants to merge 14 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds per-component arrow contexts (Popover, Tooltip, NavigationMenu), a centralized Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as Trigger Element
participant Root as Component Root (provides ArrowContext)
participant Positioner as Positioner Element
participant Hook as useArrowPosition
participant Arrow as Arrow Element
Trigger->>Root: mount / register triggerRef
Root->>Positioner: expose positionerRef via context
Note over Trigger,Positioner: ArrowContext holds refs and arrowPadding
Positioner->>Hook: request compute (triggerEl, positionerEl, side, align, offset)
Hook-->>Positioner: return computed CSS (top/left/right/bottom, transform)
Positioner->>Arrow: apply inline style (arrowStyle)
Arrow-->>Trigger: visually aligns to trigger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
MaxLee-dev
force-pushed
the
fix-popover-arrow-collision-shift
branch
from
ce8192d to
d4baf08
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/components/popover/popover.stories.tsx (1)
79-79: Translate Korean comments to English for codebase consistency.The inline comments on these lines are in Korean. For consistency and accessibility to all contributors, consider translating them:
- Line 79:
align="center" (default) + collision shift: arrow가 trigger를 따라가는지 확인- Line 168:
align="end" + collision shift- Line 215:
align="start" + collision shift✏️ Suggested translations
- {/* align="center" (default) + collision shift: arrow가 trigger를 따라가는지 확인 */} + {/* align="center" (default) + collision shift: verify arrow tracks trigger */}- {/* align="end" + collision shift */} + {/* align="end" + collision shift */}- {/* align="start" + collision shift */} + {/* align="start" + collision shift */}Also applies to: 168-168, 215-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/popover/popover.stories.tsx` at line 79, Translate the inline Korean comments into English for consistency: replace the comment containing align="center" (default) + collision shift: arrow가 trigger를 따라가는지 확인 with an English equivalent such as "align=\"center\" (default) + collision shift: verify the arrow follows the trigger", and similarly translate comments near align="end" + collision shift and align="start" + collision shift to "align=\"end\" + collision shift" and "align=\"start\" + collision shift" with brief clarifying text like "verify collision shift behavior" so contributors can easily find these comments adjacent to the Popover story examples referencing align="center", align="end", and align="start".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/components/popover/popover.stories.tsx`:
- Line 79: Translate the inline Korean comments into English for consistency:
replace the comment containing align="center" (default) + collision shift:
arrow가 trigger를 따라가는지 확인 with an English equivalent such as "align=\"center\"
(default) + collision shift: verify the arrow follows the trigger", and
similarly translate comments near align="end" + collision shift and
align="start" + collision shift to "align=\"end\" + collision shift" and
"align=\"start\" + collision shift" with brief clarifying text like "verify
collision shift behavior" so contributors can easily find these comments
adjacent to the Popover story examples referencing align="center", align="end",
and align="start".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71af0c5d-f38d-4ef5-9d40-4636aec9728a
⛔ Files ignored due to path filters (2)
.github/pr-assets/after-arrow-collision.pngis excluded by!**/*.png.github/pr-assets/before-arrow-collision.pngis excluded by!**/*.png
📒 Files selected for processing (2)
packages/core/src/components/popover/popover.stories.tsxpackages/core/src/components/popover/popover.tsx
|
✅ All tests passed!
Click here if you need to update snapshots. |
noahchoii
left a comment
noahchoii
left a comment
There was a problem hiding this comment.
These changes are intentional and reflect our current design guidelines. It might be a good idea to sync with the designer on this. Also, just as a heads-up, we are using the same custom arrow positioning across Tooltip and NavigationMenu as well to maintain a unified look.
|
Also, it seems like the visual regression tests are passing even though you haven't updated the snapshots yet...? Could you double-check this as well? |
I understood the design intent to be setting the arrow’s position with a 12px offset relative to the positioner’s location (if this is not the case, please let me know the actual intent!). Taking this intention into account, I have modified the code to utilise the 
|
As the snapshots are based on the full screen, collisionAvoidance does not trigger in this case, so there is no UI difference and the tests are passing.! In that case, it would be a good idea to add a testbed for cases where collisionAvoidance does work. Before doing so, I will add it once it has been confirmed that this change does not conflict with the existing design intent! |
MaxLee-dev
force-pushed
the
fix-popover-arrow-collision-shift
branch
from
cbc8faf to
d194931
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/components/navigation-menu/navigation-menu.tsx (1)
184-200: Consider clearingactiveTriggerElementwhen trigger closes.The
MutationObserversetsactiveTriggerElementwhendata-popup-openis present but doesn't clear it when the attribute is removed. If the stale trigger element is removed from the DOM before another trigger opens,getBoundingClientRect()may return zeros briefly.This is a minor edge case since the popup isn't visible when closed, and opening a new trigger self-corrects.
♻️ Optional fix to clear stale reference
const observer = new MutationObserver(() => { if (node.hasAttribute('data-popup-open')) { setActiveTrigger(node); + } else { + setActiveTrigger((current) => (current === node ? null : current)); } });Note: This requires changing
setActiveTriggerElementto accept a callback, or using a different approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/navigation-menu/navigation-menu.tsx` around lines 184 - 200, The MutationObserver and initial check only setActiveTrigger when data-popup-open is present but never clear it; update the observer callback (and the initial presence check) to call setActiveTrigger(null) (or undefined) when the attribute is removed so activeTriggerElement is cleared, and ensure the setter signature for setActiveTrigger (or setActiveTriggerElement) accepts null/undefined (or switch to a callback-style setter that can clear state). Reference internalRef.current, the MutationObserver, setActiveTrigger (or setActiveTriggerElement), and the data-popup-open attribute when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/hooks/use-arrow-position.ts`:
- Line 4: The Side union includes 'inline-start' and 'inline-end' which
computeArrowPosition doesn't handle; update computeArrowPosition (and its
isHorizontalSide helper) to treat 'inline-start'/'inline-end' as horizontal
sides (compute left/right positioning) or else restrict the Side type to only
'top'|'bottom'|'left'|'right'. Specifically, either expand isHorizontalSide to
return true for 'inline-start' and 'inline-end' and add branching in
computeArrowPosition to map those logical sides to concrete left/right
calculations (respecting RTL if necessary), or change the Side type declaration
to remove the inline-* values and adjust callers to only pass supported sides.
Ensure you update any tests or usages that rely on logical sides (references:
Side, computeArrowPosition, isHorizontalSide).
---
Nitpick comments:
In `@packages/core/src/components/navigation-menu/navigation-menu.tsx`:
- Around line 184-200: The MutationObserver and initial check only
setActiveTrigger when data-popup-open is present but never clear it; update the
observer callback (and the initial presence check) to call
setActiveTrigger(null) (or undefined) when the attribute is removed so
activeTriggerElement is cleared, and ensure the setter signature for
setActiveTrigger (or setActiveTriggerElement) accepts null/undefined (or switch
to a callback-style setter that can clear state). Reference internalRef.current,
the MutationObserver, setActiveTrigger (or setActiveTriggerElement), and the
data-popup-open attribute when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08f969db-5ea5-415d-9183-5597cdcfcb7a
⛔ Files ignored due to path filters (4)
packages/core/__tests__/screenshots/popover--test-bed-1-chrome-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-edge-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-firefox-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-safari-darwin-.pngis excluded by!**/*.png
📒 Files selected for processing (5)
packages/core/src/components/navigation-menu/navigation-menu.tsxpackages/core/src/components/popover/popover.stories.tsxpackages/core/src/components/popover/popover.tsxpackages/core/src/components/tooltip/tooltip.tsxpackages/core/src/hooks/use-arrow-position.ts
💤 Files with no reviewable changes (1)
- packages/core/src/components/popover/popover.stories.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/components/tooltip/tooltip.tsx (1)
123-130: Verify that arrow positioning updates correctly after initial render.The hook receives
ctx?.triggerRef.current ?? nullwhich will benullduring the initial render before refs are populated. TheuseArrowPositionhook guards against null elements (returns{}), and the subsequent state update fromextractPositionsin theuseEffectat lines 135-143 triggers a re-render with valid refs.This should work correctly, but relies on the state update cycle. Consider adding a brief inline comment explaining that the initial
useEffecttriggers a re-render that provides valid refs to the hook.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/tooltip/tooltip.tsx` around lines 123 - 130, Add a short inline comment near the useTooltipArrowContext/useArrowPosition invocation explaining that triggerRef.current and positionerRef.current may be null on first render and that the hook returns {} until extractPositions runs inside the useEffect, which updates state and triggers a re-render with populated refs; reference the useTooltipArrowContext, useArrowPosition call and the extractPositions/update effect so future readers understand the deliberate reliance on the initial state update cycle.packages/core/src/components/popover/popover.tsx (1)
135-141: Consider documenting the offset difference between popover and tooltip.Popover uses the default offset (12) while tooltip uses conditional offset (12 for top/bottom, 6 for left/right at line 129 in tooltip.tsx). If this asymmetry is intentional for visual consistency with different arrow sizes or positioner styles, a brief comment would help future maintainers understand the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/popover/popover.tsx` around lines 135 - 141, Popover's arrow positioning call (useArrowPosition in popover.tsx within the Popover component using usePopoverArrowContext) uses a default offset of 12 while Tooltip's implementation uses a conditional offset (12 for top/bottom, 6 for left/right); add a concise comment next to the useArrowPosition invocation (or above the Popover component) explaining this asymmetry and the rationale (e.g., visual consistency due to different arrow sizes/positioner styles) and explicitly mention the values (12 vs 6) and the side-based behavior so future maintainers understand why the offsets differ.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/components/popover/popover.tsx`:
- Around line 135-141: Popover's arrow positioning call (useArrowPosition in
popover.tsx within the Popover component using usePopoverArrowContext) uses a
default offset of 12 while Tooltip's implementation uses a conditional offset
(12 for top/bottom, 6 for left/right); add a concise comment next to the
useArrowPosition invocation (or above the Popover component) explaining this
asymmetry and the rationale (e.g., visual consistency due to different arrow
sizes/positioner styles) and explicitly mention the values (12 vs 6) and the
side-based behavior so future maintainers understand why the offsets differ.
In `@packages/core/src/components/tooltip/tooltip.tsx`:
- Around line 123-130: Add a short inline comment near the
useTooltipArrowContext/useArrowPosition invocation explaining that
triggerRef.current and positionerRef.current may be null on first render and
that the hook returns {} until extractPositions runs inside the useEffect, which
updates state and triggers a re-render with populated refs; reference the
useTooltipArrowContext, useArrowPosition call and the extractPositions/update
effect so future readers understand the deliberate reliance on the initial state
update cycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 657f16c5-cf01-4857-88c2-fec1e103b04d
📒 Files selected for processing (2)
packages/core/src/components/popover/popover.tsxpackages/core/src/components/tooltip/tooltip.tsx
…line-end positioning
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/components/popover/popover.stories.tsx (1)
166-199: Cover the collision-shift regression in the Test Bed.This block broadens logical-side coverage, but it still does not give you a stable visual check for the bug this PR is fixing: the arrow tracking a positioner shifted by
collisionAvoidance={{ align: 'shift' }}acrossstart,center, andendin a constrained layout. Since current CI snapshots do not trigger collision handling, I'd add that scenario here as well.As per coding guidelines "Use Storybook
Test Bedfor visual regression testing".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/popover/popover.stories.tsx` around lines 166 - 199, Add a Storybook Test Bed scenario that exercises collisionAvoidance={{ align: 'shift' }} across logical inline sides so the arrow/positioner shift regression is visually captured: update the popover stories that use Popover.Root / Popover.Trigger / Popover.Popup / Popover.PositionerPrimitive to include a constrained container (small width) inside a Test Bed and render Popover.Popup instances for side values 'inline-start', 'center', and 'inline-end' with collisionAvoidance={{ align: 'shift' }} so the positioner is forced to collide and shift; ensure the Test Bed wrapper and snapshots are used so CI will run the visual regression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/components/popover/popover.tsx`:
- Around line 135-142: The popover arrow is receiving ref snapshots
(ctx?.triggerRef.current, ctx?.positionerRef.current) which are null on first
render so useArrowPosition never recalculates; update PopoverPopupPrimitive to
mirror the ref .current values into local state (e.g., triggerElementState and
positionerElementState) using a useEffect that sets those states when
ctx?.triggerRef.current or ctx?.positionerRef.current change, then pass those
state values (not the raw ref.current) into useArrowPosition (called in this
file alongside usePopoverArrowContext and getArrowSideStyle) so the hook gets
real elements after mount and can run its recalculate callback.
In `@packages/core/src/hooks/use-arrow-position.ts`:
- Around line 88-118: The effect that calls recalculate() on side/align change
must also run when offset changes; update the useEffect dependency array that
currently lists [recalculate, side, align] to include offset as well so that
changes to offset trigger recalculate(), ensuring computeArrowPosition (used in
recalculate and referencing offsetRef.current) produces up-to-date arrow
coordinates for the Arrow positioning logic in use-arrow-position.ts.
---
Nitpick comments:
In `@packages/core/src/components/popover/popover.stories.tsx`:
- Around line 166-199: Add a Storybook Test Bed scenario that exercises
collisionAvoidance={{ align: 'shift' }} across logical inline sides so the
arrow/positioner shift regression is visually captured: update the popover
stories that use Popover.Root / Popover.Trigger / Popover.Popup /
Popover.PositionerPrimitive to include a constrained container (small width)
inside a Test Bed and render Popover.Popup instances for side values
'inline-start', 'center', and 'inline-end' with collisionAvoidance={{ align:
'shift' }} so the positioner is forced to collide and shift; ensure the Test Bed
wrapper and snapshots are used so CI will run the visual regression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b67f5fbc-8ae5-4c11-8f74-c1dcbbde262e
📒 Files selected for processing (8)
packages/core/src/components/popover/popover.css.tspackages/core/src/components/popover/popover.stories.tsxpackages/core/src/components/popover/popover.tsxpackages/core/src/components/tooltip/tooltip.css.tspackages/core/src/components/tooltip/tooltip.stories.tsxpackages/core/src/components/tooltip/tooltip.tsxpackages/core/src/hooks/use-arrow-position.test.tsxpackages/core/src/hooks/use-arrow-position.ts
💤 Files with no reviewable changes (2)
- packages/core/src/components/popover/popover.css.ts
- packages/core/src/components/tooltip/tooltip.css.ts
… for improved rendering
…ate and adjust arrow positioning logic
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/components/navigation-menu/navigation-menu.tsx (1)
183-199: Consider clearingactiveTriggerElementwhen popup closes.The observer sets
activeTriggerElementwhendata-popup-openis present but doesn't clear it when removed. While this likely doesn't cause functional issues (the arrow only renders when open), it leaves a stale reference. Consider adding an else branch for clarity.♻️ Optional improvement
const observer = new MutationObserver(() => { if (node.hasAttribute('data-popup-open')) { setActiveTriggerElement(node); + } else { + setActiveTriggerElement(null); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/components/navigation-menu/navigation-menu.tsx` around lines 183 - 199, The effect that watches internalRef.current via the MutationObserver sets setActiveTriggerElement(node) when node.hasAttribute('data-popup-open') but never clears it; update the logic in the useEffect (the block referencing internalRef, node.hasAttribute, setActiveTriggerElement, and the MutationObserver) to call setActiveTriggerElement(null) (or undefined per existing types) when the attribute is removed—both in the initial check path and inside the observer callback—so the active trigger reference is cleared when the popup closes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/components/navigation-menu/navigation-menu.tsx`:
- Around line 343-349: The arrow styling in navigation-menu.tsx is missing the
orientation/rotation from getArrowSideStyle; update the arrow style merge to
combine getArrowSideStyle(side) with the computed position returned by
useArrowPosition (which uses activeTriggerElement and positionerRef with
side/align) so the transform and axis offsets are applied, and add an import for
getArrowSideStyle at the top of the file; ensure you call
getArrowSideStyle(side) and merge its result with the existing position object
when building the arrow's style prop.
In `@packages/core/src/components/tooltip/tooltip.tsx`:
- Around line 132-146: The initial state for tooltip side is inconsistent:
change the useState default in the side state (currently in the component where
useState<TooltipPositionerPrimitive.Props['side']> is called) from 'bottom' to
'top' so it matches TooltipPositionerPrimitive's default; update any related
assumptions around useArrowPosition, getArrowSideStyle and the extractPositions
effect to rely on 'top' as the initial side so the arrow orientation is correct
on first render.
---
Nitpick comments:
In `@packages/core/src/components/navigation-menu/navigation-menu.tsx`:
- Around line 183-199: The effect that watches internalRef.current via the
MutationObserver sets setActiveTriggerElement(node) when
node.hasAttribute('data-popup-open') but never clears it; update the logic in
the useEffect (the block referencing internalRef, node.hasAttribute,
setActiveTriggerElement, and the MutationObserver) to call
setActiveTriggerElement(null) (or undefined per existing types) when the
attribute is removed—both in the initial check path and inside the observer
callback—so the active trigger reference is cleared when the popup closes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2c33da3-a453-46c2-a620-42045ab06a12
⛔ Files ignored due to path filters (8)
packages/core/__tests__/screenshots/popover--test-bed-1-chrome-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-edge-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-firefox-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/popover--test-bed-1-safari-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/tooltip--test-bed-1-chrome-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/tooltip--test-bed-1-edge-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/tooltip--test-bed-1-firefox-darwin-.pngis excluded by!**/*.pngpackages/core/__tests__/screenshots/tooltip--test-bed-1-safari-darwin-.pngis excluded by!**/*.png
📒 Files selected for processing (9)
packages/core/src/components/navigation-menu/navigation-menu.tsxpackages/core/src/components/popover/popover.css.tspackages/core/src/components/popover/popover.stories.tsxpackages/core/src/components/popover/popover.tsxpackages/core/src/components/tooltip/tooltip.css.tspackages/core/src/components/tooltip/tooltip.stories.tsxpackages/core/src/components/tooltip/tooltip.tsxpackages/core/src/hooks/use-arrow-position.test.tsxpackages/core/src/hooks/use-arrow-position.ts
💤 Files with no reviewable changes (2)
- packages/core/src/components/popover/popover.css.ts
- packages/core/src/components/tooltip/tooltip.css.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/components/tooltip/tooltip.stories.tsx
- packages/core/src/hooks/use-arrow-position.test.tsx
- packages/core/src/components/popover/popover.stories.tsx
…-popover-arrow-collision-shift
Provides a ref that always holds the latest value using useIsoLayoutEffect, preventing stale closures without destabilizing callback identities. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e timing - Merge getArrowSideStyle into useArrowPosition so callers receive a complete arrow style object - Replace manual ref assignments with useLatest for arrowDimensions - Use primitive deps (side, align, offset) directly in useCallback - Switch both effects to useIsoLayoutEffect to prevent arrow flicker - Add JSDoc to computeArrowPosition and useArrowPosition Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ation-menu Remove getArrowSideStyle import and manual style merging from callers since useArrowPosition now returns the complete arrow style. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| paddingBlock: vars.size.space[150], | ||
| paddingInline: vars.size.space[200], | ||
|
|
There was a problem hiding this comment.
The other popup elements with padding were in the popup container, but since the navigation-menu was in the content container, I moved it to the popup container to maintain consistency.
| '&[data-side="top"]': { | ||
| bottom: '-8px', | ||
| transform: 'rotate(-180deg)', | ||
| }, | ||
| '&[data-side="right"]': { | ||
| left: '-12px', | ||
| transform: 'rotate(-90deg)', | ||
| }, | ||
| '&[data-side="bottom"]': { | ||
| top: '-8px', | ||
| transform: 'rotate(0deg)', | ||
| }, | ||
| '&[data-side="left"]': { | ||
| right: '-12px', | ||
| transform: 'rotate(90deg)', | ||
| }, | ||
|
|
There was a problem hiding this comment.
We have moved the logic responsible for the arrow’s position and rotation to within use-arrow-position to align with the data-side.
The reasons are as follows:
- Layout must be determined by taking into account the positions of the positioner, arrow and trigger. As there are a total of three components (popover, tooltip and navigation-menu), we have removed the styles to ensure consistent layout across all components.
| const [activeTriggerElement, setActiveTriggerElement] = useState<Element | null>(null); | ||
| const positionerRef = useRef<HTMLElement>(null); |
There was a problem hiding this comment.
Unlike tooltips and popovers, the navigation menu contains multiple triggers within the Root. As a result, there are limitations to specifying the trigger element using a ref, so the element is managed via state.
|
|
||
| const position = useMemo(() => getArrowPosition({ side, align }), [side, align]); | ||
| const { activeTriggerElement, positionerRef } = useNavigationMenuArrowContext() ?? {}; | ||
| const arrowDimensions = { width: 16, height: 8 }; |
There was a problem hiding this comment.
As the arrow is configured differently for each component, we calculate the position by assigning a size.
| width: vars.size.dimension[100], | ||
| height: vars.size.dimension[200], | ||
|
|
||
| transform: 'rotate(180deg)', |
There was a problem hiding this comment.
As the icons for all components are positioned relative to the bottom arrow, this transform has been removed.
| paddingInline: vars.size.space['100'], | ||
| borderRadius: vars.size.borderRadius['300'], | ||
| backgroundColor: vars.color.background.contrast[200], | ||
| border: `0.0625rem solid ${vars.color.border.normal}`, |
There was a problem hiding this comment.
The figma prototype has no border.
| @@ -0,0 +1,13 @@ | |||
| import { useRef } from 'react'; | |||
There was a problem hiding this comment.
Added to ensure that object parameters, which generate new references with every render, are excluded from the deps of useCallback and useEffect, whilst still allowing the callback
to always read the latest values
noahchoii
left a comment
noahchoii
left a comment
There was a problem hiding this comment.
This seems like a much more reliable way to provide Arrows for Overlay components! Great idea.
One thing to consider, though: currently, we’re seeing a lot of boilerplate code, such as creating Contexts and calling hooks in every component just to calculate the Arrow's position. Could we perhaps create a dedicated Arrow component to encapsulate all that logic within its own scope? This would help clean up the implementation across the different components.
3 participants
Problem
arrowelement could not handle situations where a shift occurred due to static positioning.Changes