-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
Consider sass-extract or theo as a replacement for herman-export/sass-json-export. #217
Comments
@jgerigmeyer I think this would be a great question to address sooner rather than later? |
@mirisuzanne Sure, good call. I'll start here tomorrow morning. |
@mirisuzanne I see some tradeoffs here, and I'd like to talk through the pros/cons with you. Right now (with
Basically, the change would be that the user provides a path to a It would be possible to automatically parse the scss file where the annotation is found (looking there for the variable), but 1) that may not be a valid scss file on its own (i.e. it may use mixins or whatnot from elsewhere), and 2) it's not guaranteed that the file containing the annotation also contains the necessary variable/map. So I don't think that's a reliable approach. So, if we go this route, I think the current const filePath = path.resolve(env.dir, env.herman.sass.variablesfile);
const vars = sassExtract.renderSync({
file: filePath,
importer: url => {
if (url[0] === '~') {
url = path.resolve(env.dir, 'node_modules', url.substr(1));
}
return { file: url };
},
includePaths: env.herman.sass.includepaths,
}).vars;
const fontData = vars.global.$herman.value.fonts.value[fontKey].value; (Sidenote: There's actually a bug in Variables are exposed as processed objects (not just the raw values). It's not really a problem, for where right now we get:
We'd get this instead:
In any case, I'm not 100% convinced this would be better. The user would no longer need to provide a CSS file generated with I do think sass-extract (and sass-extract-loader, https://github.com/jgranstrom/sass-extract-loader) is a candidate to replace our current |
(@davisagli @wlonk No pressure, but happy for your thoughts here as well if you like.) |
that makes sense, I think. If we can't really automate more of the Sass API side, I don't have much reason to push this. |
I mean, we can automate most of it... but we still need to know where there's a valid Scss file with the maps/variables. It would certainly a less Herman-centric approach (no more custom mixin; just a path to a Scss file). I could go either way. |
Thoughts from a discussion with @mirisuzanne: Pro: It would be great to get rid of both Con: Right now, |
In case we do want to use this, PR submitted to fix the custom |
Still interested in thoughts here from @mirisuzanne @davisagli or @wlonk if anyone has a leaning one way or the other. |
I have no opinions yet. |
After reading through the above, no strong leaning. I think it would be a significant win for the user only if we make the change to remove herman-export and herman-add, because then they only have to worry about how they configure herman and not also how they write their Sass for herman. |
Yeah, I think that's a pretty compelling argument to try to make this change. And it doesn't seem unreasonable to say, "Any Scss that you're expecting Herman to understand (e.g. |
I agree. We'll want to add a map-compile feature to each Accoutrement (since our maps don't fint a standard format), but that's a good idea anyway. Make Herman more flexible, and our tooling can adjust to fit. |
Another option would be the salesforce-ux/theo approach – with design tokens in YAML… |
https://github.com/jgranstrom/sass-extract
The text was updated successfully, but these errors were encountered: