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

Allow colors to be defined as closures #1676

Merged
merged 2 commits into from Aug 15, 2020
Merged

Allow colors to be defined as closures #1676

merged 2 commits into from Aug 15, 2020

Conversation

innocenzi
Copy link
Contributor

This pull request allows colors to be defined using closures. The closure takes an opacityVariableName as a parameter, which allows to use the opacity variable's name in the color definition:

// tailwind.config.js
module.exports = {
  theme: {
    colors: {
      primary: variable => `rgba(32, 32, 32, var(${variable}, 1))`,
    },
  },
}

This allows to take advantage of the new opacity utilities. It is especially useful for theming, as seen in the example above.

This example will generate the following text and background color utilities (and other ones which I omitted for simplicity's sake):

.bg-primary {
  background-color: rgba(32, 32, 32, var(--bg-opacity, 1));
}

.text-primary {
  color: rgba(32, 32, 32, var(--text-opacity, 1));
}

This allows us to use any opacity utility with our custom color implementation:

<div class="bg-primary bg-opacity-50"></div>

This is actually needed for me to integrate the opacity utilities to tailwindcss-theming, but it would benefit anyone who wants to use a custom color implementation using CSS variables.

@AndrewBogdanovTSS
Copy link

AndrewBogdanovTSS commented May 1, 2020

@hawezo looks good to me but will transparency be polyfillable for IE11? It seems like it won't.

<div class="text-red text-opacity-50"> //No color transparency in IE11
<div class="text-red-50"> // Can be polyfilled with `postcss-custom-properties` with full transparency support

Maybe it's worth supporting 2 modes of variables generation?

@innocenzi
Copy link
Contributor Author

@AndrewBogdanovTSS It should be polyfillable - answered with details on the issue to not pollute this PR.

@adamwathan
Copy link
Member

Sorry I'm probably being dense here but am I right that the example you gave in the opening comment doesn't actually demonstrate anything that doesn't already work without adding this new functionality?

For example if you just did this in your config:

// tailwind.config.js
module.exports = {
  theme: {
    colors: {
      primary: '#202020',
    },
  },
}

...Tailwind already does the necessary conversion to rgba with the opacity property automatically under the hood, so this will work already:

<div class="bg-primary bg-opacity-50"></div>

Can you give an example of something that doesn't currently work that this feature would enable? Am I right in thinking your actual use case is something more like this:

// tailwind.config.js
module.exports = {
  theme: {
    colors: {
      primary: variable => `rgba(var(--color-primary), var(${variable}, 1))`,
    },
  },
}

If you could share your actual use case that would be helpful, thanks.

@innocenzi
Copy link
Contributor Author

innocenzi commented Jun 27, 2020

Yes sorry, I tried to simplify my example. Your last snippet describes accurately what this feature would allow me to do.

I would have the following:

:root {
  --color-primary: 31,31,31;
}

And the following class would be generated thanks to your snippet:

.text-primary {
  color: rgba(var(--color-primary), var(--text-opacity, 1));
}

.bg-primary {
  color: rgba(var(--color-primary), var(--bg-opacity, 1));
}

Which would allow to do this:

<div class="bg-primary bg-opacity-50"></div>
<div class="text-primary text-opacity-50"></div>

The difference being that the primary color can change at runtime thanks to --color-primary.

@adamwathan
Copy link
Member

I'm good with adding this but want to make sure the API is well-thought out. Specifically the variable we are passing in. Right now we're using positional arguments in this PR which could be regrettable if we ever needed to pass another value through.

I think we should pass this through as an object where the user can destructure out the pieces they need, but that means it's important that we come up with a good name for the property since the user will have to type it.

I'm also conflicted on if we should pass the variable name through or if we should pass the entire alpha declaration through, like --text-opacity vs var(--text-opacity, 1), because that will influence the property name.

If we passed var(--text-opacity, 1) through, we could just call it alpha, but if we pass --text-opacity we should probably call it something like colorOpacityCustomProperty or something awful.

Can you think of any downside to passing the whole var(--text-opacity, 1) block? Or on the flip side any nice benefits of it? If this is going to be a new public API it's very important we have no regrets about it down the road because I take breaking changes pretty seriously in Tailwind :(

@innocenzi
Copy link
Contributor Author

Good points. I'd also prefer an object to be passed, and it'll be consistent with the rest of Tailwind.

Regarding whether to pass --text-opacity or var(--text-opacity, 1), I'd prefer the former. It would allow me personally to not have 1 by default (because tailwindcss-theming has another variant system), to have another CSS variable dictate that default opacity, and basically, to use --text-opacity however I want. I really just want the name, in this case.

About the name, I can think of the following:

  • opacityVariable
  • opacityCssVariable
  • opacityProperty
  • opacityCustomProperty

I agree that naming this one might not be the easiest. opacityVariable shouldn't conflict with any future potential addition to that object, I think?

@adamwathan
Copy link
Member

I think opacityVariable works 👍 Let's switch to an object with that property and then I'm happy to merge.

@innocenzi
Copy link
Contributor Author

I did the requested changes. Thank you!

@adamwathan
Copy link
Member

Awesome thanks! @bradlc any thoughts on this, especially around how it might make your life more difficult with the IntelliSense extension?

@innocenzi
Copy link
Contributor Author

Hey hey, any chance this is going in the next release, @adamwathan?

@adamwathan
Copy link
Member

@innocenzi I'd like to but need to get the ok from @bradlc that it won't break the intellisense plugin before releasing.

@adamwathan
Copy link
Member

Eh I bet this is actually going to be totally fine the more I think about it, and it's going to be very lightly used for the first while before people really learn about it so going to merge :) thanks for the feature!

@adamwathan adamwathan merged commit 7371ea9 into tailwindlabs:master Aug 15, 2020
@innocenzi innocenzi deleted the color-closure branch August 15, 2020 13:26
@innocenzi
Copy link
Contributor Author

Ah, thank you!

@AndrewBogdanovTSS
Copy link

Thanks, @adamwathan @innocenzi

@MrX8503
Copy link

MrX8503 commented Jan 6, 2021

@innocenzi @adamwathan
Thank you for this solution!

Any idea on how this would work with theme('colors.primary')?

EDIT: theme() fixed by (#2919)

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.

None yet

4 participants