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

Generate both global styles and classes by default #71

Merged
merged 14 commits into from
Mar 2, 2022

Conversation

sanscheese
Copy link
Contributor

Porque no los dos?

giphy

@vercel
Copy link

vercel bot commented May 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tailwindlabs/tailwindcss-forms/4q1VpDwEJt6aRC9tX9GUekkZcQxa
✅ Preview: https://tailwindcss-forms-git-fork-sanscheese-feat-475f3a-tailwindlabs.vercel.app

@adamwathan
Copy link
Member

Hey! Can you explain the use case for this? If <input> is already styled, I don't really understand why you'd also want to be able to add class="form-input" to it.

@sanscheese
Copy link
Contributor Author

sanscheese commented May 13, 2021

Hey, sure. I really like the approach of covering base inputs out of the box, without the classes. But having a few edge cases where classes would help.

Have had to style some non-input elements to match inputs as placeholders (aren't just disabled inputs).

Also targeting form elements out of our direct control say from other libraries is handy if can inject the classes. (To style stripe elements it would be great if form-input could work with @apply, but pretty sure that's not possible?)

@IsraelOrtuno
Copy link

I find this helpful too for my current project. I want the default behaviour but there are some edge cases where I would have to add the styling manually. I am currently integrating TipTap editor and I want to make it look like a textarea. I'd be so convenient to have the form-textarea class available.

Just like @sanscheese says, sometimes is also useful to style other HTML components as placeholders.

@IsraelOrtuno
Copy link

Just tested and this does the trick:

    require('@tailwindcss/forms'),
    require('@tailwindcss/forms')({
      strategy: 'class'
    }),

But not sure if this it's messing up too much...

@sanscheese
Copy link
Contributor Author

sanscheese commented Jul 12, 2021

A better example of this. Ideally, an app may want to use the base strategy. Then building something like these advanced multi-select lists, may then require styling a div to look like a select list.

@fabswt
Copy link

fabswt commented Feb 17, 2022

Just a +1 to say this would indeed be useful with Stripe Elements.

Stripe Elements inserts the inputs dynamically, inside iframes, so I'm styling these as little as possible and instead styling the parent element. So it would come in handy to be able to just apply .form-input on the parent.

@adamwathan
Copy link
Member

Thanks for the helpful feedback, definitely convinced that it makes sense to support this! The main thing I'm undecided on is the API — just don't love strategy: 'both'.

Some ideas:

  1. Support an array like strategy: ['base', 'class'], which is maybe a bit nicer than adding another keyword.
  2. Always generate the classes anyways when strategy: 'base', and leave strategy: 'class' as really just an option for disabling the base rules.
  3. Do 2, but add a new boolean option for disabling the base rules so it feels more direct. Not sure on the name but conceptually something like base: false.

I think I do like the idea of both the base rules and the classes being available right out of the box without any extra configuration personally.

Any thoughts or opinions?

@IsraelOrtuno
Copy link

@adamwathan I do like the idea of having both available right out of the box, I guess is a pretty common use case to use components from third party libraries which you probably want to style just as the rest of your form components.

@thecrypticace
Copy link
Contributor

@adamwathan my suggestion here would be a combo of options 1 and 2.

  1. An undefined strategy is the same idea as ['base', 'class'] (we could even support this explicitly)
  2. We can leave base, and class untouched.

That way we support both out of the box, we have an idea for future expansion (should we ever need one — not sure if we will), and we don't affect any users who have their strategy explicitly set. Thoughts?

@reinink
Copy link
Member

reinink commented Mar 2, 2022

@thecrypticace I like this idea — where the undefined (default) strategy includes both base and class automatically, but if you'd like to force only one option to be enabled, then you can do that by explicitly setting strategy: 'base' or strategy: 'class'.

Meaning for anyone who has the strategy explicitly set right now, nothing will change. However, for anyone who doesn't have it set will now have the option to use the form-{name} classes if they want.

My only concern is this: Is anyone relying on the default strategy (base) and also creating their own form-{name} classes? And if they are, could this break things for them? I think there's a decent chance of that, but I suppose we can just release a new "major" version (v0.5.0) for those who want to opt-in to this new behavior.

@thecrypticace
Copy link
Contributor

@reinink Yep, I thought about that explicitly while I was thinking through my idea. The intent was to reduce the possibility of breakage while giving a better out-of-the-box experience. I think doing this and releasing it as v0.5.0 is the right call.

@sanscheese
Copy link
Contributor Author

sanscheese commented Mar 2, 2022

I agree with the thoughts above. I've made initial changes for this, but still just using the single string value.

I'm wondering if strategy may not be the best term for this option. If this has to wait for v0.5.0, is it worth deprecating strategy for disableLayer: 'base' (or 'components')? Think will be unlikely to need to disable more than one layer.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
Co-authored-by: Jonathan Reinink <jonathan@reinink.ca>
This is closer to the wording we use in the readme
@thecrypticace thecrypticace changed the title feat(option): allow both strategies Generate both global styles and classes by default Mar 2, 2022
@thecrypticace thecrypticace merged commit 1871f90 into tailwindlabs:master Mar 2, 2022
@thecrypticace
Copy link
Contributor

Thanks @sanscheese! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants