feat: #65 add style dictionary #120
Conversation
|
question: @willmcvay are you waiting for this to be reviewed or are there still some things you need to finish off before assigning reviewers? note: I'm happy to review it if you'd like; I'll just need to set aside a block of time to do so (it's a pretty large PR 😅). Feel free to point me at specific files if you think some of the changes don't need to be specifically reviewed 🙏 |
willmcvay
force-pushed
the
feat/#65-add-style-dictionary
branch
from
48d55dd to
fcdfb38
Compare
Hi @kurtdoherty yes, I don't want to add to it at this stage. TBH, whilst the LOC is quite high, all I have practically done is to replace the locally defined global CSS variables with those parsed from Figma to prove the concept that we can sync with the DS. I would focus on
The other files are just me replacing the old variable format with the new one e.g. Basically the same change many times :-) |
There was a problem hiding this comment.
praise: Nice work on this @willmcvay 👏 Will be great to get this in and have our CSS derived from the DS in Figma 🥳
I just had two suggestions, both of which could be deferred to separate PRs if you'd prefer.
tokens/build.mjs
Outdated
| const lowerCasedTheme = theme.toLowerCase() | ||
| return { | ||
| source: ['./tokens/tokens.json'], | ||
| format: { |
There was a problem hiding this comment.
suggestion: It seems like we could we simplify this and just use the built-in CSS format to get a CSS file, e.g. tokens.css, then import that file using a CSS @import statement in globals.ts 🤔
note: It's quite possible I'm missing something with Linaria that prevents this from working; though it seems @import statements are permitted in the global.ts as we're using it for the Source Code Pro font.
There was a problem hiding this comment.
Nice suggestion, yes, this works - have implemented and was able to get rid of the linaria parse function.
tokens/build.mjs
Outdated
| buildPath: `tokens/${lowerCasedTheme}/`, | ||
| files: [ | ||
| { | ||
| destination: 'linaria-tokens.ts', |
There was a problem hiding this comment.
note: If my suggestion above works and gets implemented, we could avoid the linaria- and typescript- prefixes on the generated files and just have tokens.css and tokens.ts files.
There was a problem hiding this comment.
Agreed, have re-named :-)
| try { | ||
| const object = JSON.parse(contents) | ||
| // Bit fragile but I only want the primitives, semantic and component variables for each theme | ||
| // Figma exports a "mode 1" that we don't need - confirmed with Andrei |
There was a problem hiding this comment.
praise: It may be a bit fragile, but at least is well-contained to this file 👌 👏
package.json
Outdated
There was a problem hiding this comment.
suggestion: This can be done separate to this PR, but while I think of it, we'll want to be able to import the typescript tokens from Elements, but I can't see them being exported. I notice that Elements isn't yet using package exports, but adding a package export to allow us to do import * as tokens from "@reapit/elements/tokens.ts" should be sufficient for us without bloating the main entry point with stuff that no one else is likely to need.
There was a problem hiding this comment.
I added this for the TS tokens - FYI, by adding a CSS export, it duplicates the CSS variables at root which we don't want. If you need the CSS variables in your external project, you can just import the main stylesheet and they will be available globally with the other classes.
There was a problem hiding this comment.
I added this for the TS tokens
I can't see where this is has been added to the manifest. Not a big deal though; we're not trying to consume the TS tokens until after we've done the purple colour change as part of the RC rollout for Console Cloud, so can come later if need be.
FYI, by adding a CSS export, it duplicates the CSS variables at root which we don't want
Yep, it's only useful for the TS tokens and only while Console Cloud is still using MUI components alongside Elements (unless other products are going to leverage them instead of the main stylesheet). Hopefully, there'll be a day in our future where we'll have zero MUI in Console Cloud and no longer need the TS tokens export at all 🤞
|
@kurtdoherty Thanks for reviewing, if you could take a look again, would be good to get this merged in. Thanks |
|
@willmcvay everything looks good to me 👌 Let's 🚢 it (assuming the "Codacy Static Code Analysis" issues don't need to be dealt with). |
Was just playing around with Codacy, it's not a thing we are supporting currently, have removed from the repo. Cheers for the review. |
Pull request checklist
Detail as per issue below (required):
fixes: #65
Based on @lilykirkwood 's initial PR, pulls in tokens from Figma, parses and injects variables into the Linaria globals object.