-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(tokens): update tokens architecture and usage #29950
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
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 good! Reviewed every single file. 😂
| // disabled, unchecked checkbox | ||
| :host(.checkbox-disabled) .native-wrapper { | ||
| border-color: globals.$ionic-color-neutral-800; | ||
|
|
||
| background-color: globals.$ionic-state-disabled; | ||
| } |
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.
Why was this removed?
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.
From what I recall it was because this seems unnecessary, as the mixin state-disabled was already being applied to the pseudo-element, which should be enough for the disabled styling:
| @include globals.disabled-state(); |
Also, I noticed this when running the tests for the first time, the checkbox failed, as the snapshot returned a grey background. I believe this was now noticeable due to the changes on the new tokens values.
Let me know if I'm missing some detail here or use-case :)
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
Co-authored-by: Maria Hutt <[email protected]>
...on/test/basic/button.e2e.ts-snapshots/button-diff-ionic-md-ltr-light-Mobile-Chrome-linux.png
Show resolved
Hide resolved
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.
LGTM

Issue number: internal
What is the new behavior?
--configprop, where we pass the path to our Ionic token script, where we generate the needed scss variables and utility-classes (by default the tokens repo tries to be as agnostic as possible, and only generates the css variables, without the prefix and added details we need on the Ionic side).This requires to run
npm installagain.Does this introduce a breaking change?