-
Notifications
You must be signed in to change notification settings - Fork 567
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
Sass variables should (optionally?) have !default
flag
#307
Comments
I like having |
I was just picturing it being an option on the format in the config—beyond that, not sure. I guess the token level makes sense for overrides, not sure about file level…are there other options/features that work at the file level? |
Oh, I think I was multi-tasking when I wrote this, I just meant "so we can choose whether or not downstream users can override the variables," full stop. I don't know if I can conjure a reason we'd want the default flag on some tokens but not others, but it doesn't seem out of the question. |
For any token definition that includes the property `themeable: true`, a `!default` will now be added in the scss/variables format. GitHub issue amzn#307
For any token definition that includes the property `themeable: true`, a `!default` will now be added in the scss/variables format. GitHub issue amzn#307
Fixes #307 For any token definition that includes the property `themeable: true`, a `!default` will now be added in the scss/variables format.
We merged in #359 onto the 3.0 branch targeting the next major release. We will keep this issue open until the release. We want to see if this flag |
Closing this issue as this is now a feature in 3.0. You can get it today with |
In the
scss/map-deep
format, the variables it generates (for use in the map) have the!default
flag:…whereas the
scss/variables
format doesn't have that:Is there a reason why one has it and the other doesn't? To me, it seems like:
!default
should be added as an option so we can choose whether or not we want them to be able to override the variables from the configuration. (I guess that's a feature request. 😄)The text was updated successfully, but these errors were encountered: