-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(themes): add ionic theme tokens #30708
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.
|
|
@brandyscarney I have some doubts about the approach, as I see no relation was kept to the outsystems-design-tokens package in the code. I was expecting the interfaces and default values would be generated based on the info provided by the outsystems-design-tokens. Even more so, if now we are using the same structure for all themes, but at least keep that connection for the Ionic one. What happens now if the UX team decides to change the scale or the palette, for example? We would need to manual review all interfaces, values and variables to match that? The advantage of tokens is the auto sync between design (figma) and code, and its different contexts (for example, the outsystems-design-tokens package is also used on the outsystems side, to make sure everything is synced, regarding css). If we are loosing that, then there's no use to call these design tokens 😄, just type-safe css structures, like https://vanilla-extract.style/ |
|
@BenOsodrac Yeah, this is definitely an open topic. For now, the plan is to move forward with all themes using the same tokens, and the mobile UI would override those design tokens in the framework on your side. The idea is that if the Outsystems design tokens changed values it would require a breaking change release on our end, which we're trying to avoid. This work is still in the early stages, and we haven't finalized the naming or process yet. I'll add this to the Confluence doc under open topics so we can discuss it further. |
Ok thanks! 👍 But would we keep the css variables with fallbacks from the token ones? |
|
@BenOsodrac I think that decision still needs to be made also. I can send you the link to the open discussion document. It is still a WIP but feel free to add topics to it or add any thoughts to existing ones! 🙂 |
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 generally looks fine to me, I just had a few comments/questions that might just need clarity rather than changes. Approving this because I don't really consider them blocking even if you want to change something related to it.
Great work!!
bed0d9b to
050b9d0
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.
- For dark palette on the basic page, can the text be black on spacing? It's a bit hard to read.
- When using dark palette with ionic theme, the header doesn't change to a darker shade like the other themes. Is that on purpose? This happens on all the previews.
...adge.e2e.ts-snapshots/badge-shape-soft-small-size-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, my comments aren't blockers. Just make sure to update the path and should be good to merge.
Done: 6f2c0c3
The My changes enable the global dark palette for the |
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, with minor suggestion
Issue number: internal
What is the current behavior?
ionictheme tokens are undefined.darkpalette does not work with theionictheme.ionictheme uses different colors thanmdandios.What is the new behavior?
ionictheme currently.ionictheme based on the existing design tokens.enabled,rippleEffect,formHighlight) from the generated CSS variables.--backgroundand--ion-background-colorfrom the globalionictheme overrides, allowing it to use the dark palette.ionictheme, ensuring all of the themes use the same named (primary, secondary, etc.) colors.build.themes, to generate the theme files.Does this introduce a breaking change?
Other information
Preview: Basic
Preview: Color
Preview: Typography
Add the URL parameters
?ionic:palette=darkor?ionic:theme={ios|md|ionic}to see the palette or theme change and pull in the associated tokens.Note: most of the theme specific tokens are not applied yet because we haven't updated the components.