Skip to content
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

Migrate all website pages to use CSS theme variables #34880

Closed
9 of 10 tasks
siriwatknp opened this issue Oct 25, 2022 · 16 comments
Closed
9 of 10 tasks

Migrate all website pages to use CSS theme variables #34880

siriwatknp opened this issue Oct 25, 2022 · 16 comments
Assignees
Labels
ready to take Help wanted. Guidance available. There is a high chance the change will be accepted umbrella For grouping multiple issues to provide a holistic view website Pages that are not documentation-related, marketing-focused.

Comments

@siriwatknp
Copy link
Member

siriwatknp commented Oct 25, 2022

We need your help!

Help us migrate the website to start using CSS theme variables!

Progress

If you are interested to contribute, check out the How-to and add your name at the end of the bullet to let us know that you are working on it.

Once you have done the migration, open a PR with the title [website] Migrate xxx page to use CSS theme variables

where "xxx" is the page that you pick.

How-to

  1. go to the page (in docs/pages/ folder) that you want to work on. Let's take docs/pages/templates.tsx as an example.

  2. Replace BrandingProvider with BrandingCssVarsProvider

    -import BrandingProvider from 'docs/src/BrandingProvider';
    +import BrandingCssVarsProvider from 'docs/src/BrandingCssVarsProvider';
    
    -<BrandingProvider>
    +<BrandingCssVarsProvider>

    Start the development server via yarn docs:dev and goto /templates/, you are likely to encounter Warning: Prop "className" did not match. Server: or something similar. This is because some components in the page is relying on the runtime calculation to change the styles based on the color mode. Those conditions must be fixed as explained in the next step!

  3. Look at each component imported on the page, e.g. TemplateHero.tsx, and find the conditional expression like this:

    <Typography
      ...
      color={(theme) => (theme.palette.mode === 'dark' ? 'primary.400' : 'primary.600')}
    >

    Then, replace the condition with theme.applyDarkStyles():

    -color={(theme) => (theme.palette.mode === 'dark' ? 'primary.400' : 'primary.600')}
    +sx={theme => ({
    +  color: 'primary.600',
    +  ...theme.applyDarkStyles({
    +    color: 'primary.400',
    +  }),
    +})}
    • Check out migration scenarios to see other patterns. If you encounter a scenario that is not in the migration patterns, feel free to open a PR and ask for help.
    • Read the implementation details about the theme.applyDarkStyles.
  4. Refresh the page, you should not see a warning coming from the TemplateHero.tsx (you might still see the warning but coming from other components on the page).

  5. Once you have fixed all the components, open a PR with the title [website] Migrate xxx page to use CSS theme variables and tag @siriwatknp as a reviewer.

Migration patterns

Here are some use cases that you might encounter in the migration process.

1. Conditional expression in the prop that's not sx

Example code:

<Typography color={theme => theme.palette.mode === 'dark' : 'grey.200' : 'grey.700'}>

Migrated code: Move the logic to sx prop:

<Typography
  sx={theme => ({
    color: 'grey.700',
    ...theme.applyDarkStyles({
       color: 'grey.200',
    })
  }))
>

2. Usage from theme.palette.*

Example code:

<Typography color={theme => theme.palette.mode === 'dark' : theme.palette.grey[200] : theme.palette.grey[700]}>

Migrated code: attach (theme.vars || theme).*:

<Typography
  sx={theme => ({
    color: (theme.vars || theme).palette.grey[700],
    ...theme.applyDarkStyles({
       color: (theme.vars || theme).palette.grey[200],
    })
  }))
>

3. Conditional expression in sx prop

Example code:

<Typography
  sx={{
    color: theme => theme.palette.mode === 'dark' : 'grey.200' : 'grey.700',
    bgcolor: theme => theme.palette.mode === 'dark' : 'grey.500' : 'grey.600',
  }}
>

Migrated code: Remove the condition and group into dark styles:

<Typography
  sx={theme => ({
    color: 'grey.700',
    bgcolor: 'grey.600',
    // `theme.applyDarkStyles()` must come later
    ...theme.applyDarkStyles({
      color: 'grey.200',
      bgcolor: 'grey.500',
    })
  })}
>

4. Conditional expression in sx prop (with nested selectors)

Example code:

const Example = styled('a')(({ theme }) => ({
  color: theme.palette.primary[700],
  '&::before': {
    width: 2,
    height: 2,
    backgroundColor: theme.palette.mode === 'dark' ? theme.palette.primary[600] : theme.palette.background.paper,
  },
  '& > span': {
    color theme.palette.mode === 'dark' ? theme.palette.primary[400] : theme.palette.primary[500],
  },
}))

Migrated code: use array:

const Example = styled('a')(
({ theme }) => ([{
  color: theme.palette.primary[700],
  '&::before': {
    width: 2,
    height: 2,
    backgroundColor: (theme.vars || theme).palette.background.paper,
  },
  '& > span': {
    color: (theme.vars || theme).palette.primary[500],
  },
},
theme => theme.applyDarkStyles({
  '&::before': {
    backgroundColor: (theme.vars || theme).palette.primary[600],
  },
  '& > span': {
    color: (theme.vars || theme).palette.primary[400],
  }
})]))

5. img switch between color modes

From StoreTemplatesBanner.tsx.

Example code:

const globalTheme = useTheme();
  const mode = globalTheme.palette.mode;
  return (
    <Image
      ref={ref}
      src={`/static/branding/store-templates/template-${mode}${Object.keys(linkMapping).indexOf(brand) + 1}.jpeg`}
      alt=""
      {...props}
    />
  );

Migrated code: use CSS content: url(...):

<Image
  ref={ref}
  src={`/static/branding/store-templates/template-light${Object.keys(linkMapping).indexOf(brand) + 1}.jpeg`}
  alt=""
  sx={(theme) =>
    theme.applyDarkStyles({
      content: `url(/static/branding/store-templates/template-dark${Object.keys(linkMapping).indexOf(brand) + 1}.jpeg)`,
    })
  }
  {...props}
/>

6. Section wrapped with <ThemeProvider theme={darkTheme}>

Remove the ThemeProvider and add data-mui-color-scheme="dark" to the next div instead.

diff --git a/docs/pages/careers.tsx b/docs/pages/careers.tsx
index b06294da66..1ae0709c22 100644
--- a/docs/pages/careers.tsx
+++ b/docs/pages/careers.tsx
@@ -476,8 +476,7 @@ function CareersContent() {
       </Container>
       {/* Next roles */}
       {nextRolesData.length > 0 ? (
-        <ThemeProvider theme={brandingDarkTheme}>
-          <Box sx={{ bgcolor: 'primaryDark.700' }}>
+          <Box data-mui-color-scheme="dark" sx={{ bgcolor: 'primaryDark.700' }}>
             <Container sx={{ py: { xs: 4, md: 8 } }}>
               <Box
                 sx={{
@@ -531,7 +530,6 @@ function CareersContent() {
               </Stack>
             </Container>
           </Box>
-        </ThemeProvider>
       ) : null}
       {/* Frequently asked questions */}
       <Container sx={{ py: { xs: 4, sm: 6, md: 8 } }}>

Implementation details

  • We want to migrate the website page by page, so we create a new provider BrandingCssVarsProvider.tsx as a replacement for the existing BrandingProvider. It is built on top of the public CssVarsProvider API specific for mui.com which contains custom theme tokens. Once a page is wrapped with the new provider, the components under it will start using CSS theme variables immediately which causes server-client inconsistency due to conditional expressions like this theme.palette.mode === 'dark' ? theme.palette.grey[300] : theme.palette.grey[700]

  • The applyDarkStyles() util is added to the theme (this is specific to mui.com, not Material UI) to ease the migration because some components need to be backward compatible for some pages that haven't been migrated. This util is straightforward as the name implies. It will decide the approach to use based on the theme, so you don't have to know if the components are rendered in which provider. All you need to do is put the styles for dark mode to the util after the default styles.
    if the component is under the new BrandingCssVarsProvider:

    theme.applyDarkStyles({
      color: (theme.vars || theme).palette.primary.main,
    })
    
    // it returns
    :where([data-mui-color-scheme="dark"]) & {
      color: var(--muidocs-palette-primary-main);
    }

    if the component is under the old BrandingProvider:

     theme.applyDarkStyles({
       color: (theme.vars || theme).palette.primary.main,
     })
     
     // it returns
     color: #ff0dc;
  • We have added a workaround to make the :where() selector works.

@siriwatknp siriwatknp added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted website Pages that are not documentation-related, marketing-focused. labels Oct 25, 2022
@mnajdova mnajdova added the umbrella For grouping multiple issues to provide a holistic view label Oct 25, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 25, 2022

It looks great. I guess we could also cover the docs pages.

@siriwatknp
Copy link
Member Author

It looks great. I guess we could also cover the docs pages.

I think the docs pages should be separated (there are architecture decisions that need to be decided) because they contain nested providers to show the default theme.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 25, 2022

I think the docs pages should be separated

@siriwatknp ok 👍.

nested providers to show the default theme.

Regarding the theme providers structure, I would be in favor of simplifying the structure:

  • _app.js -> Removing the theme provider, it would fix the playground that doesn't show the default look & feel of the components and keep the concerns isolated, e.g. for the full template pages that we host.
  • docs/pages/* -> have each page provide a Material UI theme, customized for the brand of MUI.
  • MarkdownDocs.js -> Remove all theme providers
  • Demo.js -> Remove all theme providers
  • DemoSandboxed.js -> Provide the Material UI raw theme or Joy UI raw theme depending on the product the demo is for.

@EduardoSCosta
Copy link
Contributor

EduardoSCosta commented Oct 25, 2022

Hi, I'd like to help. Can I be assigned to the product-templates page? 😃

@jesrodri
Copy link
Contributor

Hey, I would also like to help! Can you assign me a page?

@siriwatknp
Copy link
Member Author

@EduardoSCosta Thanks!
@jesrodri Assigned you for Design kits page.

Feel free to submit a PR and tag me @siriwatknp for review.

@trizotti
Copy link
Contributor

trizotti commented Oct 26, 2022

@siriwatknp , I'd love to help. Can you assign me the "Pricing" page? 😄

@brianlu2610
Copy link
Contributor

brianlu2610 commented Oct 26, 2022

@siriwatknp I would like to contribute too. Can you assign me the "About" page?

@siriwatknp
Copy link
Member Author

@trizotti @brianlu2610 Done! feel free to submit a PR.

@the-mgi
Copy link
Contributor

the-mgi commented Oct 27, 2022

Hello folks, I would like to take on careers page.

@the-mgi
Copy link
Contributor

the-mgi commented Oct 27, 2022

#34908 @siriwatknp brother, here is a draft PR for /careers/ page, but still there's a bug that Next Roles part is not showing the right colors while in light mode. if you can point me in the right direction, it'll be very helpful.
image,

as can be seen in mui.com/templates page here in testimonials section
image

@EduardoSCosta
Copy link
Contributor

Hi, me, @jesrodri and @trizotti are facing some errors, so I openned a draft PR and commented about these errors.

@siriwatknp
Copy link
Member Author

@EduardoSCosta @the-mgi For the section that is wrapped with <ThemeProvider theme={darkTheme}>, you can remove it and add data-mui-color-scheme="dark" to the next div instead.

diff --git a/docs/pages/careers.tsx b/docs/pages/careers.tsx
index b06294da66..1ae0709c22 100644
--- a/docs/pages/careers.tsx
+++ b/docs/pages/careers.tsx
@@ -476,8 +476,7 @@ function CareersContent() {
       </Container>
       {/* Next roles */}
       {nextRolesData.length > 0 ? (
-        <ThemeProvider theme={brandingDarkTheme}>
-          <Box sx={{ bgcolor: 'primaryDark.700' }}>
+          <Box data-mui-color-scheme="dark" sx={{ bgcolor: 'primaryDark.700' }}>
             <Container sx={{ py: { xs: 4, md: 8 } }}>
               <Box
                 sx={{
@@ -531,7 +530,6 @@ function CareersContent() {
               </Stack>
             </Container>
           </Box>
-        </ThemeProvider>
       ) : null}
       {/* Frequently asked questions */}
       <Container sx={{ py: { xs: 4, sm: 6, md: 8 } }}>

@the-mgi
Copy link
Contributor

the-mgi commented Oct 28, 2022

@EduardoSCosta @the-mgi For the section that is wrapped with <ThemeProvider theme={darkTheme}>, you can remove it and add data-mui-color-scheme="dark" to the next div instead.

diff --git a/docs/pages/careers.tsx b/docs/pages/careers.tsx
index b06294da66..1ae0709c22 100644
--- a/docs/pages/careers.tsx
+++ b/docs/pages/careers.tsx
@@ -476,8 +476,7 @@ function CareersContent() {
       </Container>
       {/* Next roles */}
       {nextRolesData.length > 0 ? (
-        <ThemeProvider theme={brandingDarkTheme}>
-          <Box sx={{ bgcolor: 'primaryDark.700' }}>
+          <Box data-mui-color-scheme="dark" sx={{ bgcolor: 'primaryDark.700' }}>
             <Container sx={{ py: { xs: 4, md: 8 } }}>
               <Box
                 sx={{
@@ -531,7 +530,6 @@ function CareersContent() {
               </Stack>
             </Container>
           </Box>
-        </ThemeProvider>
       ) : null}
       {/* Frequently asked questions */}
       <Container sx={{ py: { xs: 4, sm: 6, md: 8 } }}>

thanks a lot brother. it is now good 🙌🏼
#34908 here is thr PR, if there is anything you want tme to improve i'll be more than happy to help. thanks 🙌🏼
and @siriwatknp can you assign me the /x page? ofc if the previous PR seems good to you.

@EduardoSCosta
Copy link
Contributor

We are getting this CI error, and we don't no why 🤔

Screenshot_20221028_124512

@siriwatknp
Copy link
Member Author

Thanks, everyone for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to take Help wanted. Guidance available. There is a high chance the change will be accepted umbrella For grouping multiple issues to provide a holistic view website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

No branches or pull requests

8 participants