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.

Sign up for GitHub

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

Jump to bottom

[Typography] Improve custom component types support #18868

Merged
merged 1 commit into from
Dec 17, 2019
Merged

[Typography] Improve custom component types support #18868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 28 additions & 20 deletions packages/material-ui/src/Typography/Typography.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,39 @@
import * as React from 'react';
import { StandardProps, PropTypes } from '..';
import { OverrideProps, OverridableTypeMap, OverridableComponent } from '../OverridableComponent';
import { ThemeStyle } from '../styles/createTypography';

type Style = ThemeStyle | 'srOnly';

export interface TypographyProps
extends StandardProps<React.HTMLAttributes<HTMLElement>, TypographyClassKey> {
align?: PropTypes.Alignment;
color?:
| 'initial'
| 'inherit'
| 'primary'
| 'secondary'
| 'textPrimary'
| 'textSecondary'
| 'error';
component?: React.ElementType<React.HTMLAttributes<HTMLElement>>;
display?: 'initial' | 'block' | 'inline';
gutterBottom?: boolean;
noWrap?: boolean;
paragraph?: boolean;
variant?: Style | 'inherit';
variantMapping?: Partial<Record<Style, string>>;
export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'> {
props: P & {
align?: PropTypes.Alignment;
color?:
| 'initial'
| 'inherit'
| 'primary'
| 'secondary'
| 'textPrimary'
| 'textSecondary'
| 'error';
display?: 'initial' | 'block' | 'inline';
gutterBottom?: boolean;
noWrap?: boolean;
paragraph?: boolean;
variant?: Style | 'inherit';
variantMapping?: Partial<Record<Style, string>>;
};
defaultComponent: D;
classKey: TypographyClassKey;
}

declare const Typography: OverridableComponent<TypographyTypeMap>;
Copy link
Member

@rosskevin rosskevin Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This change makes the export of TypographyProps unused, does it not @fyodore82? So code that used to reference and Pick from TypographyProps I see is now broken and a mismatch for the Typography component itself.

For example:

// simple override of default variant
export const Typo = React.forwardRef<any, TypographyProps>((props, ref) => (
  <Typography variant="body2" ref={ref} {...props} />
))

        // valid
        <Typography component="a" href="url" display="block" />
        // now invalid
        <Typo component="a" href="url" display="block" />

Copy link
Member

Choose a reason for hiding this comment

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

I see this is an updated pattern elsewhere in the code (e.g. Chip, Grid, Link etc), but unsure how the *Props exports relate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pattern I've employed for Typography is used in several other components in Material-UI as you already mentioned. So I've decided not to invent different solution but follow already established mainline.

The TypographyProps and Typography itself are related. Both inherit from OverrideProps interface. Typography does it indirectly, rhru OverridableComponent and TypographyProps does it directly.

forwardRef will also work fine if specialize TypographyProps with correct component

export const Typo = React.forwardRef<any, TypographyProps<'a', {component: 'a'}>>((props, ref) => (
    <Typography variant="body2" ref={ref} {...props} />
))

// now valid
<Typo component="a" href="url" display="block" />

I have also checked how it was before Typography change. This code <Typo component="a" href="url" display="block" /> was not valid due to presence of href attribute. And we had no means to make it valid.

I may suggest to extend Typography documentation and include samples of how to use component prop in different scenarios

Copy link
Contributor

Choose a reason for hiding this comment

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

@fyodore82 So was the documentation ever updated? Because now I can't use FC<TypographyProps> anymore, which was a bit of a low blow to sneak into a minor release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it


export type TypographyProps<
D extends React.ElementType = TypographyTypeMap['defaultComponent'],
P = {}
> = OverrideProps<TypographyTypeMap<P, D>, D>;

export type TypographyClassKey =
| 'root'
| 'h1'
Expand Down Expand Up @@ -54,6 +64,4 @@ export type TypographyClassKey =
| 'displayInline'
| 'displayBlock';

declare const Typography: React.ComponentType<TypographyProps>;

export default Typography;
39 changes: 39 additions & 0 deletions packages/material-ui/src/Typography/typography.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React, { FC } from 'react';
import { Typography } from '@material-ui/core';

const TypographyTest = () => {
const CustomComponent: React.FC<{ prop1: string; prop2: number }> = () => <div />;
return (
<div>
<Typography />
<Typography align="inherit" color="inherit" display="block" />
<Typography align="left" color="initial" display="inline" />
<Typography align="right" color="primary" display="initial" />
<Typography align="justify" color="secondary" display="initial" />
<Typography align="inherit" color="textPrimary" />
<Typography align="inherit" color="textSecondary" />
<Typography align="inherit" color="error" />
// $ExpectError
<Typography display="incorrectValue" />
<Typography component="a" href="url" display="block" />
<Typography component="label" htmlFor="html" display="block" />
// $ExpectError
<Typography component="a" href="url" display="incorrectValue" />
// $ExpectError
<Typography component="a" incorrectAttribute="url" />
// $ExpectError
<Typography component="incorrectComponent" href="url" />
// $ExpectError
<Typography component="div" href="url" />
// $ExpectError
<Typography href="url" />
<Typography component={CustomComponent} prop1="1" prop2={12} />
// $ExpectError
<Typography component={CustomComponent} prop1="1" prop2={12} id="1" />
// $ExpectError
<Typography component={CustomComponent} prop1="1" />
// $ExpectError
<Typography component={CustomComponent} prop1="1" prop2="12" />
</div>
);
};