feat: Add Popover UI primitive #1849
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| /** | ||
| * Calculates the initial position of the popover based on the position prop. | ||
| */ | ||
| function getInitialPosition( |
There was a problem hiding this comment.
I don't see these functions being reused so i've left them in the Popover file
cpcramer
force-pushed
the
paul/add-popover-primitive
branch
from
99bf90e to
0de6048
Compare
cpcramer
force-pushed
the
paul/add-popover-primitive
branch
from
cd3a1fa to
009edc9
Compare
| // 2. This dismissal logic to fire (forcing close) | ||
| // This would create a race condition where the popover rapidly closes and reopens | ||
| const isTriggerClick = (event.target as HTMLElement).closest( | ||
| '[aria-label="Toggle swap settings"]', |
There was a problem hiding this comment.
No, it will be passed as a param
| switch (align) { | ||
| case 'start': | ||
| if (isVerticalPosition) { | ||
| left = triggerRect.left; | ||
| } else { | ||
| top = triggerRect.top; | ||
| } | ||
| break; | ||
| case 'center': | ||
| if (isVerticalPosition) { | ||
| left = triggerRect.left + (triggerRect.width - contentRect.width) / 2; | ||
| } else { | ||
| top = triggerRect.top + (triggerRect.height - contentRect.height) / 2; | ||
| } | ||
| break; | ||
| case 'end': | ||
| if (isVerticalPosition) { | ||
| left = triggerRect.right - contentRect.width; | ||
| } else { | ||
| top = triggerRect.bottom - contentRect.height; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Probably fine but if it gets any more complex I'd probably do this in some mapping like this since it's more declarative
const alignmentMap = {
start: {
vertical: () => ({ left: triggerRect.left }),
horizontal: () => ({ top: triggerRect.top }),
},
center: {
vertical: () => ({ left: triggerRect.left + (triggerRect.width - contentRect.width) / 2 }),
horizontal: () => ({ top: triggerRect.top + (triggerRect.height - contentRect.height) / 2 }),
},
end: {
vertical: () => ({ left: triggerRect.right - contentRect.width }),
horizontal: () => ({ top: triggerRect.bottom - contentRect.height }),
},
};
const { top, left } = {
...initialPosition,
...alignmentMap[align][isVerticalPosition ? 'vertical' : 'horizontal'](),
};
There was a problem hiding this comment.
I like it, will follow up with some cleanup here
There was a problem hiding this comment.
I'm implementing this primitive into the SwapSettings component - I'm expecting that this alignment might change a little
| const isTriggerClick = (event.target as HTMLElement).closest( | ||
| '[aria-label="Toggle swap settings"]', | ||
| ); | ||
| if (isTriggerClick) { |
There was a problem hiding this comment.
i won't block on this, but I find the name trigger click confusing. what is a trigger?
There was a problem hiding this comment.
Trigger is the button that opens the popup or dialog
| expect(popover.style.top).toBeDefined(); | ||
| expect(popover.style.left).toBeDefined(); |
There was a problem hiding this comment.
nit: seems like these should test to see if the correct values are passed in
|
|
||
| fireEvent(window, new Event('resize')); | ||
|
|
||
| expect(screen.getByTestId('ockPopover')).toBeInTheDocument(); |
There was a problem hiding this comment.
hmm, this doesn't seem like it's testing if position was updated
|
|
||
| fireEvent.scroll(window); | ||
|
|
||
| expect(screen.getByTestId('ockPopover')).toBeInTheDocument(); |
|
@cpcramer approved with some comments on the tests |
What changed? Why?
Add Popover UI Primitive.
This primitive handles positioning relative to an anchor / trigger element, focus management, click outside and escape key dismissal, portal rendering, and proper ARIA attributes.
Notes to reviewers
How has it been tested?
Tested with SwapSettings component - PR implementation to follow.
Tested updates to DismissableLayer, WalletModal still works as expected.