Skip to content

[core] Export TypographyVariant type#19598

Merged
oliviertassinari merged 4 commits into
mui:masterfrom
aleccaputo:export-typography-types
Feb 14, 2020
Merged

[core] Export TypographyVariant type#19598
oliviertassinari merged 4 commits into
mui:masterfrom
aleccaputo:export-typography-types

Conversation

@aleccaputo

@aleccaputo aleccaputo commented Feb 6, 2020 •
edited by oliviertassinari
Loading

Copy link
Copy Markdown
Contributor

for feature #19572

  • export useful TS types for typography which were hard to discover
  • Remove unused code

Closes #19572

@aleccaputo
export more types for typography
934fb17 
- export useful TS types for typography which were hard to discover
- Remove unused code
@aleccaputo
aleccaputo requested a review from eps1lon February 6, 2020 23:10
@mui-pr-bot

mui-pr-bot commented Feb 6, 2020 •
edited
Loading

Copy link
Copy Markdown

No bundle size changes comparing 01b8b09...1ed5965

Generated by 🚫 dangerJS against 1ed5965

oliviertassinari
oliviertassinari reviewed Feb 7, 2020
View reviewed changes

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Form how I understood it, the idea what to expose more variables from createMuiTheme, for instance, the ones we import deep in https://material-ui.com/guides/typescript/? I don't understand why the types of the Typography component needs to be changed.

eps1lon
eps1lon requested changes Feb 7, 2020
View reviewed changes

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this but this is not what we discussed in #19572. It was specifically about createTypography and generally about key types that can't be extracted.

Comment thread packages/material-ui/src/Typography/Typography.d.ts Outdated
export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'> {
props: P & {
align?: PropTypes.Alignment;
align?: TypographyAlignment;

@eps1lon eps1lon Feb 7, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need this as explained in #19572 (comment)

@aleccaputo aleccaputo Feb 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, will do

@aleccaputo aleccaputo Feb 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@eps1lon fixed

@aleccaputo

aleccaputo commented Feb 7, 2020

Copy link
Copy Markdown
Contributor Author

Form how I understood it, the idea what to expose more variables from createMuiTheme, for instance, the ones we import deep in https://material-ui.com/guides/typescript/? I don't understand why the types of the Typography component needs to be changed.

@oliviertassinari Ah i defiantly misunderstood that, but can do that if you would like. Would you want me to just import/export them straight from index.d.ts?

@aleccaputo
aleccaputo requested a review from eps1lon February 7, 2020 17:07
eps1lon
eps1lon requested changes Feb 8, 2020
View reviewed changes

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We were discussing Variant in createTypography not Typography. I can see no issue with Typography. Please stick to the things we discussed in the issue.

@oliviertassinari

oliviertassinari commented Feb 13, 2020

Copy link
Copy Markdown
Member

@aleccaputo Did you have the time to look at the issue? :)

@aleccaputo

aleccaputo commented Feb 13, 2020

Copy link
Copy Markdown
Contributor Author

@oliviertassinari @eps1lon how's that look?

eps1lon
eps1lon approved these changes Feb 13, 2020
View reviewed changes

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perfect, thanks!

@eps1lon eps1lon changed the title Export more types for typography [core] Export TypographyVariant type Feb 13, 2020
@oliviertassinari
oliviertassinari merged commit 9f54e27 into mui:master Feb 14, 2020
@oliviertassinari

oliviertassinari commented Feb 14, 2020

Copy link
Copy Markdown
Member

@aleccaputo Thank you for the cooperation!

This was referenced Mar 10, 2020
Merged
Merged
Merged
Closed
Closed
Closed
Open
This was referenced Mar 21, 2020
Merged
Open
This was referenced Apr 16, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes
@eps1lon eps1lon eps1lon approved these changes

Assignees

No one assigned

Labels

typescript

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Export useful developer Typescript types

4 participants

@aleccaputo @mui-pr-bot @oliviertassinari @eps1lon