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

[core,colors] fix !default modifiers in Sass variables #5260

Merged
merged 1 commit into from
Apr 15, 2022

Conversation

adidahiya
Copy link
Contributor

Changes proposed in this pull request:

Fixes the Sass variables issues I described in #5225 (comment).

The ! default modifier was accidentally removed from Sass variables in #4941. Adding back the CLI argument brings those back, both for core and colors packages.

Also I've removed the ability to override $ns, as it is meaningless to do so.

@blueprint-bot
Copy link

[core,colors] fix !default modifiers in Sass variables

Previews: documentation | landing | table | demo

@adidahiya adidahiya merged commit 44cd86b into develop Apr 15, 2022
@adidahiya adidahiya deleted the ad/fix-colors-default branch April 15, 2022 17:33
@bradchristensen
Copy link
Contributor

bradchristensen commented Apr 16, 2022

I don't think this has had the intended effect - the output files (lib/less/colors.less and lib/less/colors.scss) are still being generated without the !default flags.

I started digging and it looks like the getSassVarsSync method being used in the generate-css-variables script is just throwing away those flags, even though they're being retained first by the retainDefault flag passed into the script. cleanedInput is correct, but parsedVars is not. This actually seems to match up with the example on the get-sass-vars README where it shows the !default flag being thrown away there too: https://github.com/niksy/get-sass-vars#indexscss

It looks like this might have only become an issue quite recently in #5246 when the script was rewritten for interop with node-sass and dart-sass.

I can't see any simple fix, but perhaps the !default flags need to be merged back into parsedVars afterwards? The variable names are being tracked alongside, so default flags could be tracked with them too.

EDIT: I've opened this PR with the fix described above 🙂

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

3 participants