- Notifications
You must be signed in to change notification settings - Fork13.4k
feat(themes): enable all palettes#30874
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:ionic-modular
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
vercelbot commentedDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latest updates on your projects. Learn more aboutVercel for GitHub.
|
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.
The difference is the text color. The old version is showing#1f1f1f and the updated version is showing#000. The text color was not being displayed correctly until I updated theset-content file. If you compare the text color tonext then you can verify that it should be#000.
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.
The difference is the text color. The old version is showing#1f1f1f and the updated version is showing#000. The text color was not being displayed correctly until I updated theset-content file. If you compare the text color tonext then you can verify that it should be#000.
| // TODO(FW-4004): Implement dark mode | ||
| if(paletteName==='dark'&&theme.palette?.dark){ | ||
| theme.palette.dark.enabled='always'; | ||
| // TODO(FW-4005): Implement high contrast mode | ||
| }elseif(paletteName==='high-contrast'&&theme.palette?.highContrast){ | ||
| theme.palette.highContrast.enabled='always'; | ||
| // TODO(FW-4005): Implement high contrast dark mode | ||
| }elseif(paletteName==='high-contrast-dark'&&theme.palette?.highContrastDark){ | ||
| theme.palette.highContrastDark.enabled='always'; |
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.
We can decide at the time of dark and high contrast implementation if this order makes sense.
| customTheme:{ | ||
| palette:{ | ||
| dark:{ | ||
| enabled:'always', |
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 file is meant to be always be in dark mode as stated in the file path.
| // TODO(FW-6864): Remove once IonToolbar themes are added | ||
| toolbar:{ | ||
| background:'#1f1f1f', | ||
| }, |
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.
Tests fail when--ion-toolbar-background is not set in certain cases likemd/dark. Since the goal of this PR is to enable the palettes, I didn't think create this variable under the components since as of now it would have rendered it as--ion-components-ion-toolbar-background. I would have had to either update the generate function to not add the--ion-components suffix or update all the--ion-toolbar-background to the new variable if I wanted to do it properly. That would have been outside of the scope so this approach was taken along with adding a TODO to come back.
| // TODO(FW-6864): Remove once IonToolbar themes are added | ||
| toolbar:{ | ||
| background:'#1f1f1f', | ||
| }, |
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.
Tests fail when--ion-toolbar-background is not set in certain cases likemd/dark. Since the goal of this PR is to enable the palettes, I didn't think create this variable under the components since as of now it would have rendered it as--ion-components-ion-toolbar-background. I would have had to either update the generate function to not add the--ion-components suffix or update all the--ion-toolbar-background to the new variable if I wanted to do it properly. That would have been outside of the scope so this approach was taken along with adding a TODO to come back.
| // TODO(FW-6864): Remove once IonToolbar themes are added | ||
| toolbar?: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.
Tests fail when--ion-toolbar-background is not set in certain cases likemd/dark. Since the goal of this PR is to enable the palettes, I didn't think create this variable under the components since as of now it would have rendered it as--ion-components-ion-toolbar-background. I would have had to either update the generate function to not add the--ion-components suffix or update all the--ion-toolbar-background to the new variable if I wanted to do it properly. That would have been outside of the scope so this approach was taken along with adding a TODO to come back.
| ({ title, config, screenshot})=>{ | ||
| test.describe(title('toast: high contrast: buttons'),()=>{ | ||
| // TODO(FW-4005): Once high contrast themes are fully implemented in ionic modular, remove the skips from these tests | ||
| test.describe.skip(title('toast: high contrast: buttons'),()=>{ |
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 test fails because there arecertain styles that are not available for toast. The point of this PR is to enable the palettes to work within tests and to not meddle with any components. Those styles should be reworked when we are fully implementing high contrast into the new theming structure.
Uh oh!
There was an error while loading.Please reload this page.
Issue number: internal
What is the current behavior?
The new theming structure does not allow high contrast and high contrast dark to render properly. This can be seen when running a test that uses
set-content.What is the new behavior?
themes.Does this introduce a breaking change?
Other information
Colors preview
How to test:
setContent