-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add first class support for extending the default theme #655
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
Conversation
'20': '.2', | ||
'40': '.4', | ||
}, | ||
height: theme => theme.width, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you intentionally setting the theme.width
as the height
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, test is to verify that if you have a closure value in your user config that you can still extend that same value and have the extensions add to the user overrides 👍
src/util/resolveConfig.js
Outdated
} | ||
|
||
function mergeExtensions(theme) { | ||
return mergeWith({}, without(theme, 'extend'), theme.extend, (_, extensions, key) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not you get the same result spreading the parameter?
function mergeExtensions({extend, ...theme}) {
return mergeWith({}, theme, extend, (_, extensions, key) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, so much better, thanks! ❤️
This looks really great! I can definitely see this an improvement over the current ‘create an entire copy’ approach. One drawback I can see is for TypeScript, it can infer the values when something is splatted Basically with TypeScript, you can create a type from an object’s keys. So if you change the object, then what is allowed by that type would also change. But here it would only see the keys listed under e.g. // Import the actual tailwind.js config
import Tailwind from "../tailwind";
export type BackgroundColor = keyof typeof Tailwind.backgroundColors; Something more advanced might by possible where it is able to detect the |
Would this work if i want to prepend a font to one of the font stacks? Or would this append it instead? |
@BurntCaramel Yeah interesting thought, I don't have any experience with Typescript or any real desire to use it so I don't plan to change things drastically to accommodate it. We could probably add some way to export the user's flattened config/theme though so that could be used as a type reference? @SjorsO Right now that would actually completely replace the |
For people like me who like complete control over the classes that are generated, would this approach be anymore difficult? I actually like that the tailwind config is almost like documentation for your app, but I understand your trying to encourage referring to the official documentation rather than your own config. I suspect with this approach, you'll need to refer to both the documentation and config for any possible extensions to get the full picture. With upgrades, it'll definitely be easier as you don't need to copy and paste new/adjusted values, but I do worry tailwind will slowly keep on creeping up in size without people really knowing why... |
@adamwathan I agree that adding special cases is not a good idea. Adding a paragraph in the documentation about how to prepend a font to a font stack would probably be a better solution. |
Nah won't be any more difficult, you still have just as much control it's just that we won't generate every value and stuff them in your config file for you by default. You will still be able to get a fully-fleshed out config file if you like by doing something like
Yeah I initially was worried about this being annoying but then I realized it's already true even with Tailwind 0.x, because so many of the classes have no configuration. So if you want to know what all of the
This actually already happens, too. If we add a new module to Tailwind (like say |
@adamwathan Yes that sounds like a good solution, and could have uses outside of just TypeScript too.
One of these uses could be to use the flattened config to generate your own custom documentation! I think would be amazing to say here is my configuration, now generate my custom version of the tailwindcss.com docs. I’m happy to contribute here! |
In a way this combines PRs tailwindlabs#655 and tailwindlabs#774.
In a way this combines PRs tailwindlabs#655 and tailwindlabs#774.
One of the changes we are making for 1.0 is to start encouraging people to only replace/extend what they need to change in the config file, rather than scaffolding every single value and giving the user complete ownership by default.
A challenge with this approach is that it's not quite as simple to just add a few new values to your config (or more aptly your
theme
in 1.0 terms).Previously all of the default values would already be present, and you could just tack on any extra ones you wanted to add:
But with the approach we are recommending for 1.0, there would be no
borderWidths
key in your theme by default, and to add the two new values you'd also have to re-add the existing values.My original thinking for working around this was just adding a new export (something like
tailwindcss/defaultTheme
) that users could import and spread into their theme to extend things, for example:...but I realized this would break down in combination with the theme-values-as-closures support added in #645 (you can't spread in an object that hasn't even been fully evaluated yet).
So instead, the best idea I could come up with is a new
extend
key inside the theme section that will properly handle all of this stuff for you.It looks like this:
By handling this in a first-class way we can intelligently handle the closure situations to give you intuitive results that make perfect sense but would be almost impossible to implement ergonomically in userland.
For example, this config file adds a few custom colors to the overall color palette, and adds a new background color, with the background colors still inheriting their base values from the user-extended color palette:
The result here would be that you'd get all of the default background color utilities (
bg-blue
,bg-blue-light
, etc.), as well asbg-cyan
andbg-magenta
from the extended color palette, andbg-customBackground
for the color added only tobackgroundColors
. Doing this in user-land with thedefaultTheme
export and trying to spread stuff in yourself is not really even possible without extracting some variables to avoid duplication.With this PR, a user theme would end up being broken into two sections: complete replacements, and extensions.
I expect in the real world it would usually look something like this: