Skip to content

Conversation

@fyodor-e
Copy link
Contributor

…aryTypographyProps

fix #19036

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 10, 2020

No bundle size changes comparing 10bc98f...67c6089

Generated by 🚫 dangerJS against 67c6089

@oliviertassinari
Copy link
Member

@fyodore82 Could you have a look at the test fails? @eps1lon Is the pull request taking the right direction?

@fyodor-e
Copy link
Contributor Author

@oliviertassinari , I'll surely take a look and make correction next week, so tests will pass. I had no time yesterday so I left it as is.

@fyodor-e
Copy link
Contributor Author

Several comments for changes made.
I have removed Partial from primaryTypographyProps and secondaryTypographyProps for the following reasons:

  1. All props of Typography are already optional
  2. component prop is optional.
  3. And the main reason, if component is used, OverrideProps will add component own props. Some components, especially custom, may have required props. With Partial all props, even required by custom component, will become optional which may introduce bugs. See line 23 of ListItemText.spec.tsx as example

@eps1lon
Copy link
Member

eps1lon commented Jan 13, 2020

Thank you for starting this effort. There are a lot of moving parts so reviewing this might take some time. First look is promising though

@benneq

This comment has been minimized.

@rart
Copy link
Contributor

rart commented Mar 15, 2020

Note this PR now addresses the underlying issue for all affected components at once (e.g. CardHeader is breaking too).

@eps1lon @fyodore82

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Nice work. Just had to update the branch and made some stylistic changes.

@eps1lon eps1lon merged commit 4fe9051 into mui:master Mar 18, 2020
@rart
Copy link
Contributor

rart commented Mar 18, 2020

Whats the plan for CardHeader? It’s pretty much same issue. Is anyone working on that already? Happy to transfer this work to CardHeader if it helps. If there’s any special considerations let me know too. @eps1lon @fyodore82

@fyodor-e
Copy link
Contributor Author

@rart , yes, for CardHeader solution will be the similar.
I think you can transfer it. At least I am not working on CardHeader.

@oliviertassinari oliviertassinari added the scope: list Changes related to the list label Mar 19, 2020
@oliviertassinari oliviertassinari changed the title [ListItemText] Add comonent prop to primaryTypographyProps and second… [List] Fix component types of ListItemText Mar 19, 2020
@oliviertassinari
Copy link
Member

@rart 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: list Changes related to the list typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ListItemText secondaryTypographyProps component error

6 participants