-
Notifications
You must be signed in to change notification settings - Fork 205
feat: Add modes to visual context and reference token theming definitions #4085
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
Conversation
| tokenCategories.forEach(({ tokens, mode: modeId, referenceTokens }) => { | ||
| const mode = modes.find(mode => mode.id === modeId); | ||
| if (referenceTokens) { | ||
| builder.addReferenceTokens(referenceTokens, mode); |
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.
Looks like the mode argument is not supported here, so you need to modify this function to make it work
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.
Yeah, that is the theming-core PR that's mentioned in the description.
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.
How did you test this? Did you link the packages locally?
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.
I linked the packages locally and ran them in my dev-pipeline. There is also an internal counterpart to this here: CR-238953785
style-dictionary/utils/index.ts
Outdated
| }, {} as StyleDictionary.ExpandedMotionScopeDictionary); | ||
| }; | ||
|
|
||
| export const expandReferenceTokens = (referenceTokens: any) => { |
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.
I'd replace any with a more specific type here. This function clearly expects an object with a color prop here
7374c59 to
23e230d
Compare
23e230d to
3df2ad7
Compare
c199534 to
d4b1a80
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4085 +/- ##
=======================================
Coverage 97.12% 97.12%
=======================================
Files 864 864
Lines 25327 25327
Branches 9141 9140 -1
=======================================
Hits 24599 24599
- Misses 681 722 +41
+ Partials 47 6 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This adds an internal prop to our visual context definition that gets forwarded to our theme builder in order to support multi-mode color generation for reference tokens.
Some contexts use dark mode tokens in light mode (top navigation, content header), this happens in the style-dictionary folder of this package. However, color generation of reference palettes from seed values doesn't happen until the tokens are processed in the theming-core package. While by default there will be a single palette generated that's used for both light and dark mode, someone could define two separate palettes for reference tokens, and the theme needs to know which one to use.
Related links, issue #, if available: SIM: D351729488, Internal counterpart CR-238953785
This change needs theming PR to be merged to pass dryrun.
How has this been tested?
dev-pipeline: AwsUi-v3-jkuelz
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.