search

besteadfast / carbon

Steadfast's starter project - Craft CMS, Vite, and DDEV
3 stars 2 forks source link

Modal Component #63

Open btbunze opened 2 weeks ago

btbunze commented 2 weeks ago

62

jakedohm commented 1 week ago

This looks great! I pushed a few small tweaks we talked about @btbunze.

2 Larger Considerations:

Classes

This currently has some baked-in CSS classes: modal-background, modal-container, maybe more. I'm not sure we should included these, as I think we should prefer Tailwind styling versus custom classes. We should either 1) remove these or 2) think through them better for naming, and where they should be (should every element get a class, etc.)

Using Headless UI directly

Currently when using the component, you do this:

<Modal>
  <DialogTitle>Confirm the thing</DialogTitle>
  <DialogDescription>Please confirm your choice!</DialogDescription>
</Modal>

I have 2 concerns with this:

  1. Naming: It feels icky to have a Modal, but then use a DialogTitle. Which is it, a Modal or a Dialog? I think Modal is the correct naming but these are in conflict.
  2. Breakable API: If we ever want to use something different from Headless UI, we would either have to have the exact same API as their components, or new code would look a good bit different from legacy code.

Potential solution: Wrap headless UI components in a thin wrapper, which for now just renders the Headless UI elements. So <DialogTitle> would get wrapped by <ModalTitle>.

That would change the usage to look like this:

<Modal>
  <ModalTitle>Confirm the thing</ModalTitle>
  <ModalDescription>Please confirm your choice!</ModalDescription>
</Modal>
btbunze commented 1 week ago

@jakedohm thanks for the feedback! I think both of those recommendations are solid, and I've implemented them in the latest commit, but here are my thoughts on each of them individually:

  1. My intention with these was to identify portions of the component whose purpose might not be immediately obvious to a new developer. They aren't custom CSS classes, but rather classes used in the traditional sense of marking an element as part of a class of elements that could appear in multiple places in the document. I can see how that's unclear when they are followed by utility classes that serve no other purpose than to style the element, so I agree with removing them. I think some information on what's what in a component could be useful though, but I've been working with it for a bit now, so I can't guess the perspective of someone looking at it for the first time.
  2. Good looks! Since it's an internal library, I didn't really see a problem with a misnomer or slight change to API if we were to stop using headlessUI, but I think the extra components are a reasonable price to pay for that bit of extra consistency, and I think that's a good rule of thumb to have for future components.

Also, I don't see the close button changes you made, but once those are committed, this should be good to merge 🎉