CreateTheme Breaks overrides

JahnoelRondon commented 1 year ago

Duplicates

[X] I have searched the existing issues

Latest version

[X] I have tested the latest version

Steps to reproduce 🕹

Steps:

Create an css file to override mui classes example:

.css-1hgf8cx-MuiInputBase-root-MuiOutlinedInput-root .MuiInputBase-input {
  color: var(--text);
}

create a theme
change typography fontfamily

Current behavior 😯

I currently Have a mui theme for my typography and am currently trying to add font family.

export const theme = createTheme({
  palette: {
    primary: blue,
    secondary: deepPurple,
  },
  typography: {
    allVariants: {
      color: "var(--text)",
      // breaks overrides
      fontFamily: "var(--fontFamilyTitle)",
    },
  },
});

Expected behavior 🤔

The intended functionality is to change the font family for my typography which it does...however for some reason adding a font family to my theme for typography breaks my override css.

vs without the font family in theme

Context 🔦

I currently have root css variables that are used when overriding mui classes for certain things like input box border colors etc, I went to change the font family in create theme and my overrides no longer work. I went into the Inspector to look at the styles applied to the input that I override and all the sudden my override.css isnt even one of the styles that are applied at all.

Additionally I am following this stack overflow thread https://stackoverflow.com/questions/48319372/changing-font-family-of-all-mui-components.

For some reason if I make the font family like this.

      fontFamily: `"Roboto", "Helvetica", "Arial", sans-serif`,

I have no issues, I really dont understand what the problem is.

JahnoelRondon commented 1 year ago

I've essentially been forced to move my css code regarding mui styles to the createTheme component which I dont like. It would be nice to have the option to use a css style sheet to override without createTheme breaking this.

export const theme = createTheme({
  palette: {
    primary: blue,
    secondary: deepPurple,
  },
  typography: {
    allVariants: {
      color: "var(--text)",
      // breaks overrides
      fontFamily: "'Montserrat', sans-serif",
    },
  },
  // Overrides
  components: {
    // Form
    // Text Input
    MuiInputLabel: {
      styleOverrides: {
        root: {
          color: "var(--text3)",
          [`&.${formLabelClasses.focused}`]: {
            color: "var(--primaryHighLight)",
          },
        },
      },
    },
    MuiInputBase: {
      styleOverrides: {
        root: {
          color: "var(--text)",
        },
      },
    },
    MuiOutlinedInput: {
      styleOverrides: {
        root: {
          [`& .${outlinedInputClasses.notchedOutline}`]: {
            borderColor: "var(--text2)",
          },
          [`&:hover .${outlinedInputClasses.notchedOutline}`]: {
            borderColor: "var(--text3)",
          },
          [`&.${outlinedInputClasses.focused} .${outlinedInputClasses.notchedOutline}`]:
            {
              borderColor: "var(--primaryHighLight)",
            },
        },
      },
    },
    // hamburger drop down
    MuiList: {
      styleOverrides: {
        root: {
          backgroundColor: "var(--containerBG)",
        },
      },
    },
    MuiMenuItem: {
      styleOverrides: {
        root: {
          "&:hover": {
            backgroundColor: "var(--primaryHighLight)",
          },
        },
      },
    },
    // Bread crumbs
    MuiBreadcrumbs: {
      styleOverrides: {
        separator: {
          color: "var(--text)",
        },
      },
    },
    // Modal (delete)
    MuiDialog: {
      styleOverrides: {
        paper: {
          backgroundColor: "var(--containerBG)",
          border: "1px solid var(--primaryHighLight)",
        },
      },
    },
    // Button
    MuiButton: {
      styleOverrides: {
        root: {
          "&.Mui-disabled": {
            color: "var(--text4)",
            backgroundColor: "var(--secondaryBG)",
          },
        },
      },
    },
  },
});

mj12albert commented 1 year ago

@JahnoelRondon The CSS injection order must be flipped in order for your custom CSS to have precedence over Material UI styles:

<StyledEngineProvider injectFirst>
  <ThemeProvider theme={theme}>
    <App />
  </ThemeProvider>
</StyledEngineProvider>

Here is a working sandbox: https://codesandbox.io/s/https-github-com-mui-material-ui-issues-37043-523gyn?file=/src/App.tsx

Cristy94 commented 1 year ago

I have the right injection order, yet the fontFamily override doesn't do anything. It worked in v4, doesn't work in v5.

Cristy94 commented 1 year ago

I have the right injection order, yet the fontFamily override doesn't do anything. It worked in v4, doesn't work in v5.

After hours of debugging I found this answer: https://github.com/mui/material-ui/issues/35939#issuecomment-1406913737

There is a bug: font overrides don't work when extending a theme. Only the typography changes in the first root theme are considered, if you do createTheme(theme, {typography: fontFamily: 'Arial'}) it won't work.

Updated demo showing the issue of extended theme fonts not working properly: https://codesandbox.io/s/https-github-com-mui-material-ui-issues-37043-forked-4ul3sb?file=/src/App.tsx

raegen commented 1 month ago

I have the right injection order, yet the fontFamily override doesn't do anything. It worked in v4, doesn't work in v5.

After hours of debugging I found this answer: #35939 (comment)

There is a bug: font overrides don't work when extending a theme. Only the typography changes in the first root theme are considered, if you do createTheme(theme, {typography: fontFamily: 'Arial'}) it won't work.

Updated demo showing the issue of extended theme fonts not working properly: https://codesandbox.io/s/https-github-com-mui-material-ui-issues-37043-forked-4ul3sb?file=/src/App.tsx

Indeed. Theme typography is created (with createTypography) with typography value from the base options and then any additional option passed are applied to that created typography. The problem is that createTypography uses its root styles as baseline for variants (which let me state makes PERFECT sense and is elegant, so don't change it!). Rather, it would be preferable to repeat the typography creation cycle, and herein I suspect lies the problem and the reason this works the way it works. One of the often cited usage examples involves using Theme and ThemeOptions interchangeably, which simplifies the process immensely. The complication is the fact that createTheme is not bijective. ThemeOptions -> Theme ~~Theme -> ThemeOptions~~ Or to put it simply, for an existing instance of Theme you cannot compute the original options used to create it. So "fixing" this behaviour would involve persisting and exposing a readonly clone of the original args it was constructed with so they can be used if/when that theme instance is passed to createTheme etc. Which, as you can imagine, has its own set of implications and complications that will ultimately take incomparably more effort than it would take you to just deep merge the options and pass them to createTheme instead of relying on createTheme to merge them the way you want.

Edit: It's actually mentioned in the docs - https://mui.com/material-ui/customization/theming/#api

mui / material-ui