VE-252 Combine dark and light themes for mui v6 #1009
Conversation
…/github.com/IPG-Automotive-UK/react-ui into enhancement/ve-252-combine-theme-for-mui-v6
…combine-theme-for-mui-v6
m4manjesh
changed the title
src/ToggleColorMode/index.ts
Outdated
| @@ -1,7 +1,7 @@ | |||
| import ToggleColorMode from "./ToggleColorMode"; | |||
|
|
|||
| export type ToggleColorModeProps = { | |||
| mode?: "light" | "dark"; | |||
| mode?: "light" | "dark" | "system" | undefined; | |||
There was a problem hiding this comment.
mode?: "light" | "dark" | "system"; no need to add undefined as mode is optional.
src/SvgIcons/VirtoLogo/VirtoLogo.jsx
Outdated
| @@ -25,5 +25,5 @@ VirtoLogo.propTypes = { | |||
| /** | |||
| * styles applied to the svg element | |||
| */ | |||
| sx: PropTypes.object | |||
| sx: PropTypes.func | |||
There was a problem hiding this comment.
if this is changed to func. The VirtoLogo story also needs to be updated. But like we discussed to migrate VirtoLogo component to typeScript that will align types.
src/ThemeProvider/ThemeProvider.tsx
Outdated
| @@ -86,154 +197,21 @@ const defaultTheme: ThemeOptions = { | |||
| } | |||
| }, | |||
| typography: { | |||
| allVariants: { | |||
| color: `var(--ipg-palette-text-primary)` | |||
There was a problem hiding this comment.
typography: {
allVariants: {
color: var(--ipg-palette-text-primary),
fontFamily: "Montserrat"
}
}
fontFamily: "Montserrat" should be inside allVariants to matchstyling like before.
There was a problem hiding this comment.
For an unknown reason the Chip component was failing to run this font family if i put this inside allVariants
There was a problem hiding this comment.
Reviewed code changes since my last review. I'll leave the review of UI to @Sowbhagya-ipg.
| sx={theme => ({ | ||
| color: "#9e9e9e", | ||
| position: "absolute", | ||
| right: 12, | ||
| top: 12, | ||
| ...theme.applyStyles("dark", { | ||
| color: "#ffffff" | ||
| }) | ||
| })} |
There was a problem hiding this comment.
Why are you introducing more hardcoded colours? Lets not add to the problem.
There was a problem hiding this comment.
Aligned with @Sowbhagya-ipg to go with default action.active for icon buttons
src/MultiText/MultiText.jsx
Outdated
| @@ -41,7 +41,12 @@ export default function LabelSetter({ onChange = () => {}, rows = [] }) { | |||
| headerName: "Action", | |||
| renderCell: params => ( | |||
| <IconButton | |||
| color="primary" | |||
| sx={theme => ({ | |||
| color: "#9e9e9e", | |||
There was a problem hiding this comment.
Why the hardcoded colour?
src/MultiText/MultiText.jsx
Outdated
| @@ -98,7 +103,12 @@ export default function LabelSetter({ onChange = () => {}, rows = [] }) { | |||
| /> | |||
| <Box> | |||
| <IconButton | |||
| color="primary" | |||
| sx={theme => ({ | |||
| color: "#9e9e9e", | |||
There was a problem hiding this comment.
Why the hardcoded colour?
| | "light" | ||
| | "dark"; | ||
|
|
||
| console.log(defaultMode); |
| // render the component | ||
| render( | ||
| <ThemeProvider theme={mode}> | ||
| <ThemeProvider theme={defaultMode}> |
There was a problem hiding this comment.
We shouldn't pass the mode into ThemeProvider. The original behaviour of this test was that it passed no props in and the ThemeProvider read localStorage itself.
|
Spacing between the characters is reduced everywhere @m4manjesh Just to show you an example. |
The Icon button color looks different here. @m4manjesh . I would suggest to keep the same color action.active in both themes. You can also do the same for the similar issue found by @mattcorner and use the same action.active
|
|
I noticed that when selecting Radio button and Switch Fields options, the labels turn primary. This is an existing issue. It's up to you whether to fix it or create a separate ticket. 
|
|
The rest looks fine @m4manjesh |
|
@Sowbhagya-ipg Feedbacks addressed, please re-review. |
User menu font is not montserrat. @m4manjesh |
The number inside the step circle here is not montserrat @m4manjesh
|
|
@nikitharani @mattcorner Can you please review and approve if you are happy? |
Closes TD-252
I also think this partially fixes an dark and light toggle sync issue with storybook we found here in
VirtoAppLayout. This PR sync mode switch from storybook toVirtoAppLayoutbut not reverse.Changes
This PR continues migrating RUI into MUI v6, primarily focusing on enabling cssVariables and cssColorSchemes to keep up with industry standards and for an improved developer experience.
ThemeProvider
defaultTheme,darkThemeandlightThemetomainThemewithcolorSchemesAPI--muidefault variable prefix to--ipgto label the theme brand. WithcssVariablesenabled anyone can easily check where the tokens come from now before it just showed the color code used.See an example of how the background-color token displays now in dev tools
Before
After
Themeinterface to supportcolorSchemesThemProviderchildreninside a newChildWrappercomponent to make use ofuseColorSchemeThemeContext.tsxwhich used to support use ofuseThemehook throughout the app to get and set the current theme, instead of using the newuseColorSchemecoming with MUI v6. This includes fixes in tests as well.var(--ipg-palette...)from MUI (providing the intented color)ToggleColorMode
useColorSchemeto get and set the themeVirtoAppHeader
VirtoAppHeader.stories.tsxupdated as we no longer needed to passmodeonColourModeChangeprop was removed and tests were removed as we no longer pass the propsmodeSome other file changes
ConfirmProvider.test.tsxVirtoLogo.jsxprop to stop throwing warnings by passing sx with new theme conversion type as functionVirtoAppLayoutnow usedvar(--ipg-palette-background-paper)to enable correct background.UI/UX
N/A
Testing notes
Author checklist
Before I request a review:
I have populated the deploy-preview with relevant test data.I have tagged a UI/UX designer for review if this PR includes UI/UX changes.