ui icon indicating copy to clipboard operation
ui copied to clipboard

feat(sheet): add sheet component

Open damianricobelli opened this issue 2 years ago • 12 comments

I found it interesting to add variants to the Dialog component to allow the user to add a type prop to DialogContent.

With this, you can define whether you want it to be dialog, drawer-top, drawer-bottom, drawer-left and drawer-right. The size will be defined by the user by adding the necessary tailwind cases to the DialogContent.

https://user-images.githubusercontent.com/22398603/216096226-01442b55-9600-4881-bbb7-019ba8312e8a.mov

damianricobelli avatar Feb 01 '23 16:02 damianricobelli

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
example-playground ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 6:12PM (UTC)
next-template ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 6:12PM (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 6:12PM (UTC)

vercel[bot] avatar Feb 01 '23 16:02 vercel[bot]

Someone is attempting to deploy a commit to a Personal Account owned by @shadcn on Vercel.

@shadcn first needs to authorize it.

vercel[bot] avatar Feb 01 '23 16:02 vercel[bot]

If @shadcn likes it, I could add the necessary documentation along with the examples 🥳

damianricobelli avatar Feb 01 '23 16:02 damianricobelli

@damianricobelli Do you think it would be better if the Drawer is moved to a separate component? It’s just that somehow it’s more correct separately, but the fact is that the styles are almost all the same and can be fine together. Dilemma for me 🤔

its-monotype avatar Feb 03 '23 00:02 its-monotype

@its-monotype I also thought about it, but as you say, 90% of the code is the same, so it seemed convenient to take it as a prop rather than a new component.

Another option could be to separate it but sharing all the possible code that has the Dialog component, and only placing independently the styles of the Drawer and renaming each composite component.

Dilemma for both of us haha

damianricobelli avatar Feb 03 '23 00:02 damianricobelli

@damianricobelli Maybe it's better to name that component "Sheet" instead of "Drawer" as I understand "Drawer" is about navigation more and "Sheet" is more generic, take a look on that: 🤔Drawer or Sheets? https://link.medium.com/GtFKzaBx6wb, https://evergreen.segment.com/components/side-sheet or Radix UI names it as "Sheet" in own design system: https://design-system.modulz-deploys.com/#sheet (it's another dilemma 🤣, it's really difficult to distinguish them so maybe it's just matter of preference).

Let's return to discussion about separation. I like the idea of separating that components but sharing styles of dialog comp for sheet comp. I think it's more proper to keep them as separate components cause all design systems does that and also it will be easier to document them, and in my opinion it's easier to treat them as separate components. Now imagine that we need to separate that to individual package. We can take inspiration from here a bit: https://github.com/chakra-ui/chakra-ui/tree/main/packages%2Fcomponents%2Fmodal%2Fsrc. We could keep Dialog and Sheet in one directory and name it as "modals". I think that we can add to it "AlerDialog" too. And we can take the "Dialog" styles as foundation of "Sheet" and "AlertDialog" (https://github.com/joe-bell/cva "Composing Components" or maybe just but them into constant if we don't need a variant for base dialog styles (idk)).

its-monotype avatar Feb 03 '23 01:02 its-monotype

@its-monotype

I really like the reasons why you think it is better to separate them and at the same time use the component composition provided by the cva library. I'll go that way in the next few days and submit the necessary changes to leave all this well computerized 🙌.

Agree also about changing the name👌.

My plan, taking your comments, is to be able to unify the styles that would be shared by both Dialog, AlertDialog and Sheet, and in each case use the cva component composition pattern to extend the styles that depend on each other.

I can think of two options for sharing styles between them:

  1. Use the same idea you propose of everything in the same directory called 'modals'.
  2. To avoid creating a directory containing all 3 and to be able to continue with the "everything in the same file" structure, I can take Dialog as default, and then AlertDialog and Sheet import the default styles contained in Dialog.

What do you think?

damianricobelli avatar Feb 03 '23 02:02 damianricobelli

@damianricobelli Sounds wonderful. You can try in one file, but the only thought is that it can get very large and then it might be better to split it into several files in one directory. I would probably do it in the modals directory. Maybe it will become clearer which way to use in the process of creation. Thanks for the quick feedback btw 🫶 I hope you can do it in the best way 🙌

its-monotype avatar Feb 03 '23 02:02 its-monotype

@damianricobelli Just chiming in here to +1 what @its-monotype said.

  • I can definitely see a need for a drawer component and agree on making it a separate one.
  • It can build on top of <Dialog /> like <AlertDialog /> does and use cva for variants.
  • Let's go with <Sheet /> (we're going to keep the same naming convention as Modulz - maintainer of Radix UI).
  • OK to keep everything in one file (same as the other components) at components/ui/sheet.

shadcn avatar Feb 03 '23 05:02 shadcn

@its-monotype @shadcn Thank you both for the feedback! In the next week I will be working on this feature to bring this new component to life and organise it as you have proposed 🙌

damianricobelli avatar Feb 04 '23 01:02 damianricobelli

@shadcn @its-monotype I have just uploaded the following:

  • The structure of the Dialog component as it is in main is maintained.
  • Added a sheet.tsx file that is built by extending the Radix dialog primitive, without making use of styles that come from the dialog.tsx file component. I thought this for better scalability in the future.
  • Added documentation with examples where the position and size props are applied in <SheetContent />.

What do you think?

damianricobelli avatar Feb 06 '23 12:02 damianricobelli

@damianricobelli This is looking great! I'll review and we can release this week.

shadcn avatar Feb 07 '23 09:02 shadcn