-
Notifications
You must be signed in to change notification settings - Fork 209
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
build: token processing updates #5004
build: token processing updates #5004
Conversation
|
3c6a732
to
27dd0c7
Compare
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Pull Request Test Coverage Report for Build 12793875349Details
💛 - Coveralls |
27dd0c7
to
6bb929e
Compare
Tachometer resultsCurrently, no packages are changed by this PR... |
6bb929e
to
b8192bd
Compare
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.
This looks good!! Thanks for working on it.
Is there a possibility of incorporating the changes from #4999 into this PR? Or are these things that should not get addressed together? |
717720f
to
d392b20
Compare
93ad92a
to
f05de1e
Compare
@@ -14,7 +14,7 @@ parameters: | |||
# 3. Commit this change to the PR branch where the changes exist. | |||
current_golden_images_hash: | |||
type: string | |||
default: a095ed1ef88686fdb24936dad763598ac7981193 | |||
default: 93ad92a2fc031db0277fee635434955e438e94a9 |
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.
VRTs showed a fix in the checkbox which is now picking up the correct corner radius for Spectrum 2 🥳
7a16da6
to
535c7bd
Compare
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.
Thank you for landing this! I left a small comment about a JSDoc, but it’s non-blocking.
362be0a
to
bc101e6
Compare
Co-authored-by: Rúben Carvalho <[email protected]>
bc101e6
to
8fe05ed
Compare
Description
Main file for review:
scripts/generate-tokens-wrapper.js
tools/styles/(tokens|tokens-v2)
folders before generating asssetscustom-*.css
assets from import because that content is already concatenated in the(dark|light|medium|large)-vars.css
files.component-vars.css
in thetools/styles/tokens*
directories tosystem-theme-bridge.css
for improved clarity.Along for the ride
tasks/process-spectrum.js
removed the chalk reference and as such, also had to resolve the lint errors in order to push up that change (removed unused variables reported and replaced ts-ignore with ts-expect-error)tasks/css-tools.js
Motivation and context
By sourcing the system theming from the components, it lets us ensure the component theming provided matches the version of the component the SWC library supports. Previously, CSS was copying the theming assets from the component packages and bundling them in the token package to streamline their use by SWC. Now that we're making this update though, we can harden that pipeline to ensure the correct system mappings are being used and allowing SWC the freedom to rely on older versions of some components than others might be at while still advancing the tokens library.
How has this been tested?
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.