-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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 TypographyStyle not allowing media queries and allowing unsafe undefined access #19269
Conversation
Style is too generic and can be confused with a `style` object
No bundle size changes comparing 020b746...9427b27 |
…safe undefined access mui#19269
9a2a9d2
to
312e1db
Compare
}, | ||
}); | ||
|
||
// $ExpectType string | undefined |
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.
Was $ExpectType string
before this change which would be to narrow given the above theme.
}, | ||
}; | ||
|
||
export default function ResponsiveFontSizes() { |
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.
CustomResponsiveFontSizes
?
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.
Should be, yes. It has little impact though
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.
Yeah, it's a minor detail. I have included in the next batch of small changes. I wonder if we shouldn't change the approach: 1. either rename all the demos, to, say 'Demo' or 2. test the exported name and throw.
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.
cc @mbrookes
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.
I tend to think that giving a nice name to the demo doesn't matter and that we can normalize it (option 1).
|
||
export default function ResponsiveFontSizes() { | ||
return ( | ||
<div> |
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.
Do we need a div?
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.
Copied from the demo belowe
@@ -73,7 +74,7 @@ const theme = createMuiTheme({ | |||
<ThemeProvider theme={theme}> | |||
<CssBaseline /> | |||
{children} | |||
</ThemeProvider> | |||
</ThemeProvider>; |
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 ;
sounds off
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.
It's from prettier. If the semicolon is "off" then so is the code.
Addresses #19263 (comment)
Just illustrating that it wasn't typed. Working on a fix.