feat(ui): Popover #1696
feat(ui): Popover #1696
Conversation
|
Visit the preview URL for this PR (updated for commit f8fa2c6): https://penumbra-ui-preview--pr1696-feat-prax-156-popov-940s7bhy.web.app (expires Thu, 22 Aug 2024 08:43:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1 |
| side?: RadixPopoverContentProps['side']; | ||
| align?: RadixPopoverContentProps['align']; |
There was a problem hiding this comment.
thought: This is prob fine, but for what it's worth, I'm a little wary of giving consumers too much control.
I feel like it's generally better to reduce the number of props we expose, and try to restrict usage to the designs Sam gives us. That said, I can see how users might need the side/alignment to be different for layout reasons, so this is fine.
There was a problem hiding this comment.
In this case, I couldn't figure out how to control the position on our side. Sam wrote in Figma that it should open "on the left side if above the trigger" and "on the right side if below the trigger". Radix doesn't allow to set it dynamically - we can choose only one option. So that's why I decided to give this level of control
packages/ui/src/Popover/index.tsx
Outdated
| const Content = ({ children, side, align }: PopoverContentProps) => { | ||
| return ( | ||
| <RadixPopover.Portal> | ||
| <RadixPopover.Content sideOffset={4} side={side} align={align} asChild> |
There was a problem hiding this comment.
Could we have the content grow out of its origin, like I'm doing with the tooltip here?
There was a problem hiding this comment.
Also, could we use theme spacing values, rather than hard-coding the 4? You can do that either of two ways:
- Call
useTheme(), then passtheme.spacing(1)here, instead of4. - Use styled-components'
.attrs()method to setsideOffsettoprops.theme.spacing(1), like I'm doing here.
There was a problem hiding this comment.
Thanks, updated to use useTheme. And agree that we should use the same animation in similar components. It would be beneficial if you move the scaleIn in a shared file to reuse it between Tooltip, Popover, and other components
Closes prax-wallet/prax#156
Implements Popover component but not the MenuItem that can be nested in the Popover – will do it in a separate PR.
Check it here: https://penumbra-ui-preview--pr1696-feat-prax-156-popov-940s7bhy.web.app/?path=/docs/ui-library-popover--docs