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

[core] Fix mixins not being assignable as JSS styles #19491

Merged
merged 1 commit into from
Jan 31, 2020
Merged

[core] Fix mixins not being assignable as JSS styles #19491

merged 1 commit into from
Jan 31, 2020

Conversation

ririvas
Copy link
Contributor

@ririvas ririvas commented Jan 31, 2020

This pull request reverts one of the changes made in #19263 and addresses #19362 .

The intent of original pull request was to avoid using the CSS-in-JS (JSS) implementation specific "CSSProperties" type where the simpler React.CSSProperties can be used. However, createMixins does directly rely on ability to specify media query nested selectors, a feature not present in React.CSSProperties

This change allows us to continue to override the toolbar implementation in Typescript. Whereas it is supported in vanilla JS.

const theme = createMuiTheme({
        mixins: {
            toolbar: {
                background: '#fff',
                minHeight: 36,
                '@media (min-width:0px) and (orientation: landscape)': {
                    minHeight: 24
                },
                '@media (min-width:600px)': {
                    minHeight: 48
                }
            },
        }
    });

@ririvas
Copy link
Contributor Author

ririvas commented Jan 31, 2020

This change will fix an error a couple users reported when attempting to use theme.mixins.toolbar as a CSSProperties object.

const useStyles = makeStyles(theme => ({
    appBarSpacer: theme.mixins.toolbar,
}));

However, in #19263, the following change was made to allow React.CSSProperties to be assigned to the internal type CSSProperties.

export interface CSSProperties extends BaseCSSProperties {
  // Allow pseudo selectors and media queries
  // `unknown` is used since TS does not allow assigning an interface without
  // an index signature to one with an index signature. This is to allow type safe
  // module augmentation.
  // Technically we want any key not typed in `BaseCSSProperties` to be of type
  // `CSSProperties` but this doesn't work. The index signature needs to cover
  // BaseCSSProperties as well. Usually you would use `BaseCSSProperties[keyof BaseCSSProperties]`
  // but this would not allow assigning React.CSSProperties to CSSProperties
  [k: string]: unknown | CSSProperties;
}

In other words, while this pull request will "fix" the issue. The change in withStyles.d.ts may not have worked as expected. I'm pointing this out for reference.

Should this pull request be accepted, I'm happy to expand further if need be.

@mui-pr-bot
Copy link

No bundle size changes comparing bdebdfd...4595e49

Generated by 🚫 dangerJS against 4595e49

@eps1lon eps1lon added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material typescript labels Jan 31, 2020
@eps1lon eps1lon changed the title createMixins revert change to React.CSSProperties [core] Fix mixins not being assignable as JSS styles Jan 31, 2020
@eps1lon eps1lon merged commit 4cda143 into mui:master Jan 31, 2020
@eps1lon
Copy link
Member

eps1lon commented Jan 31, 2020

@ririvas Much appreciated, thanks!

@Methuselah96
Copy link
Contributor

@eps1lon Will this be released in a patch release?

@eps1lon
Copy link
Member

eps1lon commented Feb 1, 2020

@eps1lon Will this be released in a patch release?

It will be in the next release. Whether this will be a patch or feature release I do not know.

@lookfirst
Copy link
Contributor

Thanks for the fast fix, this bit me. =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants