Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Adzouz
added
pr: feature
fix: update lodash to match cms version
markkaylor
left a comment
markkaylor
left a comment
There was a problem hiding this comment.
Hey thanks for the time invested in this. I've left a few comments mostly about favoring composition.
I'm wondering though did you consider just using vaul? I just read https://emilkowal.ski/ui/building-a-drawer-component and I feel like it solves a lot of the common problems with drawers (including animations) and already exposes a clean API. So wondering if like we do with radix we could adapt it to our design.
| const Overlay = styled(Dialog.Overlay)<{ $isVisible: boolean }>` | ||
| background: ${(props) => setOpacity(props.theme.colors.neutral800, 0.2)}; | ||
| position: fixed; | ||
| inset: 0; | ||
| z-index: ${(props) => props.theme.zIndices.overlay}; | ||
| will-change: opacity; | ||
|
|
||
| @media (prefers-reduced-motion: no-preference) { | ||
| ${({ $isVisible, theme }) => | ||
| $isVisible | ||
| ? css` | ||
| animation: ${ANIMATIONS.fadeIn} ${theme.motion.timings[OPENING_ANIMATION_DURATION]} | ||
| ${theme.motion.easings.authenticMotion} forwards; | ||
| ` | ||
| : css` | ||
| animation: ${ANIMATIONS.fadeOut} ${theme.motion.timings[CLOSING_ANIMATION_DURATION]} | ||
| ${theme.motion.easings.easeOutQuad} forwards; | ||
| `} | ||
| } | ||
| `; |
There was a problem hiding this comment.
Couldn't overlay be a composable component. I think that would also eliminate the need for any sort of isOverlayVisible prop. The consumer either composes with or they don't. That's how Radix UI does it in their example for dialog:
and vaul
| <ToggleButton | ||
| withTooltip={false} | ||
| label={open ? toggleLabelResolved.collapse : toggleLabelResolved.expand} | ||
| onClick={() => onOpenChange(!open)} | ||
| aria-expanded={open} | ||
| > | ||
| <ToggleIconWrapper $expanded={open} aria-hidden> | ||
| <CaretDown width="1.2rem" height="1.2rem" /> | ||
| </ToggleIconWrapper> | ||
| </ToggleButton> |
There was a problem hiding this comment.
This to me looks like consumer code. The API gives me a Trigger and that should be enough for me to compose a "visible header" with whatever I want (title with a toggle) in this case. The consumer can decide to create a reusable header that they can pass to both Trigger and Header. This could eliminate the props: headerVisible and customToggleButton
const MyVisibleHeader = () => <div>coucou</div>
<Drawer.Root>
<Drawer.Trigger>
<MyVisibleHeader />
</Drawer.Trigger>
<Drawer.Content direction="right">
<Drawer.Header hasClose={hasClose} hasToggle={hasToggle}>
<MyVisibleHeader />
</Drawer.Header>
<Drawer.Body>
<p>Drawer content goes here.</p>
</Drawer.Body>
</Drawer.Content>
</Drawer.Root>| <Close> | ||
| {customCloseButton ?? ( | ||
| <IconButton withTooltip={false} label={closeLabel}> | ||
| <Cross /> | ||
| </IconButton> | ||
| )} | ||
| </Close> |
There was a problem hiding this comment.
Couldn't this be a composable component that accepts children. This would eliminate hasClose and customCloseButton as props
| <Close> | |
| {customCloseButton ?? ( | |
| <IconButton withTooltip={false} label={closeLabel}> | |
| <Cross /> | |
| </IconButton> | |
| )} | |
| </Close> | |
| const Close = ({ children }) => { | |
| if (children) return children | |
| return <IconButton withTooltip={false} label={closeLabel}> | |
| <Cross /> | |
| </IconButton> | |
| } |
Similar design to radix ui dialog:
|
Putting it on hold for the moment to improve the component API with Mark comments |
|
3 participants
What does it do?
Adds a Drawer component that can be used for multiple purposes:
Inspired by https://vaul.emilkowal.ski/
Components.Drawer.Side.Bottom.Storybook.Jan.28.2026.mp4
Why is it needed?
This component will need to be integrated a few places in the CMS.
How to test it?
-> Play with the settings to change the behaviour
🚀