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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import React from 'react';
import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';

const theme = createMuiTheme();

theme.typography.h3 = {
fontSize: '1.2rem',
'@media (min-width:600px)': {
fontSize: '1.5rem',
},
[theme.breakpoints.up('md')]: {
fontSize: '2rem',
},
};

export default function ResponsiveFontSizes() {
return (
<div>
<ThemeProvider theme={theme}>
<Typography variant="h3">Responsive h3</Typography>
</ThemeProvider>
</div>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import React from 'react';
import { createMuiTheme, ThemeProvider } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';

const theme = createMuiTheme();

theme.typography.h3 = {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
fontSize: '1.2rem',
'@media (min-width:600px)': {
fontSize: '1.5rem',
},
[theme.breakpoints.up('md')]: {
fontSize: '2rem',
},
};

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).

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

<ThemeProvider theme={theme}>
<Typography variant="h3">Responsive h3</Typography>
</ThemeProvider>
</div>
);
}
20 changes: 12 additions & 8 deletions docs/src/pages/customization/typography/typography.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const theme = createMuiTheme({
To self-host fonts, download the font files in `ttf`, `woff`, and/or `woff2` formats and import them into your code.

⚠️ This requires that you have a plugin or loader in your build process that can handle loading `ttf`, `woff`, and
`woff2` files. Fonts will *not* be embedded within your bundle. They will be loaded from your webserver instead of a
`woff2` files. Fonts will _not_ be embedded within your bundle. They will be loaded from your webserver instead of a
CDN.

```js
Expand All @@ -48,7 +48,8 @@ const raleway = {
local('Raleway-Regular'),
url(${RalewayWoff2}) format('woff2')
`,
unicodeRange: 'U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF',
unicodeRange:
'U+0000-00FF, U+0131, U+0152-0153, U+02BB-02BC, U+02C6, U+02DA, U+02DC, U+2000-206F, U+2074, U+20AC, U+2122, U+2191, U+2193, U+2212, U+2215, U+FEFF',
};
```

Expand All @@ -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.

```

## Font size
Expand All @@ -100,6 +101,7 @@ const theme = createMuiTheme({
The computed font size by the browser follows this mathematical equation:

![font-size](/static/images/font-size.gif)

<!-- https://latex.codecogs.com/gif.latex?computed&space;=&space;specification&space;\frac{typography.fontSize}{14}&space;\frac{html&space;font&space;size}{typography.htmlFontSize} -->

### HTML font size
Expand All @@ -124,7 +126,7 @@ html {
}
```

*You need to apply the above CSS on the html element of this page to see the below demo rendered correctly*
_You need to apply the above CSS on the html element of this page to see the below demo rendered correctly_

{{"demo": "pages/customization/typography/FontSizeTheme.js"}}

Expand All @@ -136,17 +138,19 @@ You can use [media queries](/customization/breakpoints/#api) inside them:
```js
const theme = createMuiTheme();

theme.typography.h1 = {
fontSize: '3rem',
theme.typography.h3 = {
fontSize: '1.2rem',
'@media (min-width:600px)': {
fontSize: '4.5rem',
fontSize: '1.5rem',
},
[theme.breakpoints.up('md')]: {
fontSize: '6rem',
fontSize: '2.4rem',
},
};
```

{{"demo": "pages/customization/typography/CustomResponsiveFontSizes.js"}}

To automate this setup, you can use the [`responsiveFontSizes()`](/customization/theming/#responsivefontsizes-theme-options-theme) helper to make Typography font sizes in the theme responsive.

{{"demo": "pages/customization/typography/ResponsiveFontSizesChart.js", "hideHeader": true}}
Expand Down
8 changes: 4 additions & 4 deletions packages/material-ui/src/Typography/Typography.d.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from 'react';
import { StandardProps, PropTypes } from '..';
import { OverrideProps, OverridableTypeMap, OverridableComponent } from '../OverridableComponent';
import { ThemeStyle } from '../styles/createTypography';
import { Variant as ThemeVariant } from '../styles/createTypography';

type Style = ThemeStyle | 'srOnly';
type Variant = ThemeVariant | 'srOnly';

export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'> {
props: P & {
Expand All @@ -20,8 +20,8 @@ export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'>
gutterBottom?: boolean;
noWrap?: boolean;
paragraph?: boolean;
variant?: Style | 'inherit';
variantMapping?: Partial<Record<Style, string>>;
variant?: Variant | 'inherit';
variantMapping?: Partial<Record<Variant, string>>;
};
defaultComponent: D;
classKey: TypographyClassKey;
Expand Down
20 changes: 8 additions & 12 deletions packages/material-ui/src/styles/createTypography.d.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Palette } from './createPalette';
import { CSSProperties } from './withStyles';

export type ThemeStyle =
export type Variant =
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
| 'h1'
| 'h2'
| 'h3'
Expand Down Expand Up @@ -31,24 +31,20 @@ export interface FontStyleOptions extends Partial<FontStyle> {
allVariants?: CSSProperties;
}

export type TypographyStyle = Required<
Pick<CSSProperties, 'fontFamily' | 'fontSize' | 'fontWeight' | 'fontStyle' | 'color'>
> &
Partial<Pick<CSSProperties, 'letterSpacing' | 'lineHeight' | 'textTransform'>>;

export interface TypographyStyleOptions extends Partial<TypographyStyle> {}
// TODO: which one should actually be allowed to be subject to module augmentation?
// current type vs interface decision is kept for historical reasons until we
// made a decision
export type TypographyStyle = CSSProperties;
export interface TypographyStyleOptions extends TypographyStyle {}

export interface TypographyUtils {
pxToRem: (px: number) => string;
}

export interface Typography
extends Record<ThemeStyle, TypographyStyle>,
FontStyle,
TypographyUtils {}
export interface Typography extends Record<Variant, TypographyStyle>, FontStyle, TypographyUtils {}

export interface TypographyOptions
extends Partial<Record<ThemeStyle, TypographyStyleOptions> & FontStyleOptions> {}
extends Partial<Record<Variant, TypographyStyleOptions> & FontStyleOptions> {}

export default function createTypography(
palette: Palette,
Expand Down
15 changes: 15 additions & 0 deletions packages/material-ui/src/styles/createTypography.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { createMuiTheme } from '@material-ui/core/styles';

{
// properties of the variants can be "unset"
const theme = createMuiTheme({
typography: {
allVariants: {
fontStyle: undefined,
},
},
});

// $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.

const maybeFontStyle = theme.typography.body1.fontStyle;
}
4 changes: 2 additions & 2 deletions packages/material-ui/src/styles/responsiveFontSizes.d.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Theme } from './createMuiTheme';
import { Breakpoint } from './createBreakpoints';
import { ThemeStyle } from './createTypography';
import { Variant } from './createTypography';

export interface ResponsiveFontSizesOptions {
breakpoints?: Breakpoint[];
disableAlign?: boolean;
factor?: number;
variants?: ThemeStyle[];
variants?: Variant[];
}

export default function responsiveFontSizes(
Expand Down