Feat/192 popover #212
Feat/192 popover #212
Conversation
![review-bot-for-github[bot]](https://avatars.githubusercontent.com/u/1544687?s=60&v=4){kind=small}
Updated the example 👍 |
There was a problem hiding this comment.
I think it's not good practice to have those high level selectors. I removed them alongside the negative margin and it looks fine to me. If you could center the separator as well rather than it being at the start that would be good.
Above is an image of the popover when I remove those selectors and margin.
src/components/Popover/index.tsx
Outdated
|
|
||
| const popoverVariants = cva([ | ||
| 'z-50', | ||
| 'border border-[2px] border-border-subtle dark:border-border-dark', |
There was a problem hiding this comment.
You can change this to border-2
src/components/Popover/index.tsx
Outdated
| className={cn( | ||
| 'shrink-0 bg-border-subtle dark:bg-border-dark', | ||
| orientation === 'horizontal' | ||
| ? 'h-[1px] w-fill -mx-4' |
There was a problem hiding this comment.
Remove this margin I think. We should find a better way of overflowing.
src/components/Popover/index.tsx
Outdated
| {...props} | ||
| > | ||
| <ScrollArea.Root className="w-full h-full" type="auto"> | ||
| <ScrollArea.Viewport className="w-full h-full gap-3 [&>*>*]:py-2 [&>*>*]:px-4"> |
There was a problem hiding this comment.
After some thought, let's not use these selectors for the padding. Let's let the user do it. This has quite a high specificity, and so will be hard for them to overwrite
There was a problem hiding this comment.
Yes when you put it like this I think flexibility is more important than how it looks out of the box, plus it keeps the component simpler
src/components/Popover/index.tsx
Outdated
| 'shrink-0 bg-border-subtle dark:bg-border-dark', | ||
| orientation === 'horizontal' | ||
| ? 'h-[1px] w-fill -mx-4' | ||
| : 'h-fill w-[1px]', |
There was a problem hiding this comment.
This separator is also inline filled. We should have it centered.
Adds a popover component
Screen.Recording.2023-12-06.at.15.57.02.mov
closes: #192