search

robinmetral / svelte-micromodal

Svelte wrapper around Micromodal to create light, accessible modal dialogs. Docs 👇
https://svelte-micromodal.pages.dev
MIT License
5 stars 2 forks source link

Added class merging for elements that had style props #9

Closed jon-robertson closed 6 months ago

jon-robertson commented 6 months ago

Adds support for tailwind users as well as anyone that wants to use classes.

robinmetral commented 6 months ago

Hey @Jon-Robertson, thanks for taking the time to open a PR!

Before adding a new feature, I'd like to better understand where this is coming from. What's the issue you're running into? Why weren't the built-in styling options (described in the docs) sufficient? Is there something that prevents Tailwind users from simply passing the classes as e.g. containerStyles?

Also, svelte-micromodal shouldn't assume Tailwind usage, so if we need to improve how styling works, I'd prefer to avoid tailwind-merge. (Instead we can probably write it ourselves by concatenating classes, no need for complex logic I think. But I might be missing something, looking forward to hearing about your use case.)

Thanks again!

P.S. I haven't used Tailwind for a while, which might explain my blind spot here. Thanks in advance for some extra context!

jon-robertson commented 6 months ago

Hey Robin. The HTML style attribute can only contain CSS declarations, not classes. Tailwind works by giving the user predefined classes which need to be added to an element's class attribute. There is of course the option of picking apart the class and manually entering each declaration in the style attribute but this would get messy when trying to do any moderate amount of styling. This isn't just an issue for Tailwind either, anyone that wants to add a class they have defined would be unable to apply it to the modal as well. I used Tailwind merge because it's what I'm used to but since we're just taking in a string of classes I don't see an issue with string concatenation instead.

robinmetral commented 6 months ago

:facepalm: I thought we were already exposing classes, just realized it's just styles. Apologies!

Then this makes a LOT of sense. If you don't mind I'd still prefer a "manual" concatenation over a new lib. Would you be okay with making the changes to your branch?

Thanks!

jon-robertson commented 6 months ago

Hi Robin, I've added a commit to remove Tailwind merge. Let me know if you'd like anything else changed and thanks for creating this repo.

robinmetral commented 6 months ago

Hi @Jon-Robertson, sorry for the late reply here and thanks a lot for this! It looks good to me — I'll go ahead and merge it, and handle the release in a few hours max. I'll send an updated here when the new version is live.

Thanks for the contribution and thanks for bearing with me!

robinmetral commented 6 months ago

Released to NPM under version 0.0.10, please let me know if you find any issues!

Note: when trying this out, I realized that using classes might be a little bit tricky out of the box because of Svelte's behavior and specificity issues.

See https://github.com/robinmetral/svelte-micromodal/issues/10 for details! In the meantime, passing custom classes to the modal should work, with caveats.