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 TypographyStyle not allowing media queries and allowing unsafe undefined access #19269

Merged
merged 4 commits into from
Jan 17, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 17, 2020

Addresses #19263 (comment)

Just illustrating that it wasn't typed. Working on a fix.

Style is too generic and can be confused with a `style` object
@eps1lon eps1lon added docs Improvements or additions to the documentation typescript labels Jan 17, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Jan 17, 2020

No bundle size changes comparing 020b746...9427b27

Generated by 🚫 dangerJS against 9427b27

@eps1lon eps1lon changed the title [docs] Add TypeScript demo for custom responsive font sizes [core] Fix TypographyStyle not allowing media queries and allowing unsafe undefined access Jan 17, 2020
@eps1lon eps1lon added package: material-ui Specific to @mui/material and removed docs Improvements or additions to the documentation labels Jan 17, 2020
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Jan 17, 2020
},
});

// $ExpectType string | undefined
Copy link
Member Author

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.

@eps1lon eps1lon merged commit 05c9004 into mui:master Jan 17, 2020
@eps1lon eps1lon deleted the test/responsive-font-size branch January 17, 2020 12:09
},
};

export default function ResponsiveFontSizes() {
Copy link
Member

@oliviertassinari oliviertassinari Jan 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CustomResponsiveFontSizes?

Copy link
Member Author

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

Copy link
Member

@oliviertassinari oliviertassinari Jan 17, 2020

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@oliviertassinari oliviertassinari Jan 17, 2020

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>
Copy link
Member

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?

Copy link
Member Author

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>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ; sounds off

Copy link
Member Author

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.

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

Successfully merging this pull request may close these issues.

5 participants