Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add strategy option to configure globals-only and classes-only #6

Merged
merged 14 commits into from
Nov 13, 2023

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Sep 8, 2023

Hi there! The https://github.com/tailwindlabs/tailwindcss-forms plugin by default applies global styles and scans for specific form-* classes to apply styles, and also accepts a strategy option to configure using classes-only or globals-only.

This PR adds the same option here, and also removes the dependency on mini-svg-data-uri and improves runtime performance by precomputing and then hard coding the resulting data URLs (this part is contained in the last commit, feel free to drop it if you prefer to keep it as-is).

I've published the contents of this branch to https://www.npmjs.com/package/@lewisl9029/unocss-preset-forms for testing, and it seems to be working as intended so far. Feel free to try it out yourself as well, and let me know if there's anything you'd like me to change. Cheers!

P.S. The diffs seem to get a lot cleaner with whitespace-hiding enabled: https://github.com/Julien-R44/unocss-preset-forms/pull/6/files?w=1

@Julien-R44
Copy link
Owner

Hey, thanks for your PR and sorry for sitting on it for so long!

I'm not sure that's something we want for the following two reasons:

  • I tend to agree with the tailwind team when they say that preset-forms should be seen as a form reset and not as a collection of classes to be applied
  • This has made the code quite complex. Honestly, it has become difficult to read

However, I'm not totally opposed to adding this feature. On the other hand, I think we really have to make the code cleaner to add it

I think we can also do without the performance optimisations on mini-svg-data-uri. We don't have a performance problem in the first place, so I tend to say, why bother optimising it? I always prefer readability and simplicity to performance when it's not critical

I'm sorry if I'm being boring, especially as I've kept you waiting for so long

@lewisl9029
Copy link
Contributor Author

Hey Julien, no worries on the timing, and appreciate the feedback!

Re: making the code harder to read. Totally agree, but I'm struggling a bit to see how to make it better. Most of the changes here I see as necessary to enable the functionality. Do you have any specific suggestions on changes that would make this easier to read for you? Happy to implement anything you suggest. :)

Re: the svg optimizations. Happy to drop the commit as mentioned. Though for context, I'm using UnoCSS to implement Tailwind compatibility for my app (https://reflame.app), instead of using Tailwind directly, precisely for it's performance advantage, and nothing else. So performance is actually critical for my use case. Though I totally appreciate that the changes here make the library harder to maintain, so I'm happy to move it over to a personal fork and take the maintenance burden off your shoulders. :)

@Julien-R44
Copy link
Owner

Julien-R44 commented Nov 12, 2023

Cool! About the optimization thing : let's keep it. We'll just separate this out in another file

As for the rest, and the fact that the code isn't very readable: I'm going to play around a bit with the PR between tonight and tomorrow and make some quick changes

Thanks again

@Julien-R44
Copy link
Owner

Julien-R44 commented Nov 13, 2023

I've worked a bit and it looks a bit cleaner now ( in my opinion at least 😅). But I've realised that we've got some problem with the class mode.

Since we're using dynamic rules, if we have the same class several times as a UnoCSS rule, it will create the same matcher (regex) twice in a row. And only the last one created will be applied.

The problem is visible on the playground, on the select element ( form-select ) if you want to take a look.

My first lazy idea was to apply the class mode styles as a preflight: so add them no matter what, even if they're not matched on the project. After all, there's not a lot of css code.

Otherwise we will have to do a bit of tinkering with the code to make it work as expected

@Julien-R44
Copy link
Owner

Should be all good now. Gonna merge and release it now

Please feel free to give any feedback if you encounter any issue!
Thanks again for the PR

@Julien-R44 Julien-R44 merged commit 78fa692 into Julien-R44:main Nov 13, 2023
3 checks passed
@Julien-R44 Julien-R44 added the enhancement New feature or request label Nov 13, 2023
@lewisl9029
Copy link
Contributor Author

Thank you so much for catching the bug with multiple dynamic rules! Was able to repro on my side too.

Just upgraded to the new version in my app and it's working perfectly. :)

@lewisl9029 lewisl9029 deleted the configure-global-or-classes branch November 13, 2023 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants