-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(themes): add md theme tokens and update ionic theme to include semantic tokens #30734
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
The latest updates on your projects. Learn more about Vercel for GitHub.
|
--ion-font-family: Roboto, "mdTestingFont", sans-serif; | ||
font-family: Roboto, "mdTestingFont", sans-serif; | ||
} | ||
|
||
/* TODO(FW-6744): Remove this after adding the ios tokens */ |
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 thought I could remove these but Playwright still needs the iosTestingFont
and mdTestingFont
.
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 does it need it?
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.
It doesn't apply Roboto without the testing font in this file:
--ion-font-family: Roboto, "mdTestingFont", sans-serif;
and it doesn't apply the apple font without this either:
--ion-font-family: -apple-system, BlinkMacSystemFont, "iosTestingFont", sans-serif;
I thought maybe this code was outdated but it is still needed so I left it. We could look into removing it later.
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.
That's weird, I wonder why it needs it. Thanks!
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.
Just noticed a weird inconsistency, but it may be intentional. Otherwise looks good to me.
core/src/themes/md/dark.tokens.ts
Outdated
}, | ||
|
||
textColorStep: { | ||
50: '#f33f33', |
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.
So this is weird, but I noticed all of the other colors here seem to be a grey color (#xyxyxy
) and this one doesn't follow that pattern, so I looked at it and this one is red. Was that intentional?
Maybe you meant #f3f3f3
? That would be grey like the others.
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.
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 current behavior?
md
themeionic
theme redefines the base numeric tokensWhat is the new behavior?
config
values into a separateconfig
option which uses theIonicConfig
interfaceformHighlight
toIonicConfig
md
themeionic
theme and adds semantic tokens--ion-font-family
overrides in favor of the tokensDoes this introduce a breaking change?