Skip to content

Conversation

@tomasznguyen
Copy link
Contributor

@tomasznguyen tomasznguyen commented Apr 8, 2021

Related to #24405

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 8, 2021

Details of bundle changes

@material-ui/lab: parsed: +0.11% , gzip: +0.07%

Generated by 🚫 dangerJS against 94d4a52

@oliviertassinari oliviertassinari added the scope: tree view Changes related to the tree view. This includes TreeView, TreeItem. label Apr 8, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

A first quick review. In packages/material-ui-lab/src/TreeView/TreeView.d.ts, Theme needs to be imported from @material-ui/core/styles.

tomasznguyen and others added 3 commits April 8, 2021 23:05
Co-authored-by: Olivier Tassinari <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]>
@tomasznguyen
Copy link
Contributor Author

@oliviertassinari Tnx for the quick review. I fixed the imports. I'll have a look at the other errors tomorrow.

@tomasznguyen
Copy link
Contributor Author

@oliviertassinari I got stuck with migrating TreeView. When running yarn proptypes, the properties className, onBlur, onFocus, and onKeyDown are removed from TreeView.propTypes.

When replacing (for testing):

const TreeView = React.forwardRef(function TreeView(inProps, ref) {
  const { isRtl, ...props } = useThemeProps({ props: inProps, name: 'MuiTreeView' });

by:

const TreeView = React.forwardRef(function TreeView(props, ref) {

yarn proptypes adds the 4 properties to TreeView.propTypes.

I try to understand why this is happening, but till now without success. Do you have any suggestion for me where to look at?

@mnajdova
Copy link
Member

mnajdova commented Apr 9, 2021

@oliviertassinari I got stuck with migrating TreeView. When running yarn proptypes, the properties className, onBlur, onFocus, and onKeyDown are removed from TreeView.propTypes.

When replacing (for testing):

const TreeView = React.forwardRef(function TreeView(inProps, ref) {
const { isRtl, ...props } = useThemeProps({ props: inProps, name: 'MuiTreeView' });
by:

const TreeView = React.forwardRef(function TreeView(props, ref) {
yarn proptypes adds the 4 properties to TreeView.propTypes.

I try to understand why this is happening, but till now without success. Do you have any suggestion for me where to look at?

Taking a look.

@mnajdova
Copy link
Member

mnajdova commented Apr 9, 2021

@tomasznguyen fixed the proptypes by c295e40

tomasznguyen and others added 2 commits April 9, 2021 22:07
@oliviertassinari oliviertassinari force-pushed the migrate-treeview-emotion branch from 3a97614 to d099b7b Compare April 9, 2021 22:01
tomasznguyen and others added 2 commits April 10, 2021 08:52
Co-authored-by: Marija Najdova <[email protected]>
@tomasznguyen
Copy link
Contributor Author

@mnajdova @oliviertassinari Thank you very much for your feedback and reviews. All tests are passing now.

Based on your reviews, I reviewed my other migration PRs. I see that index.d.ts of those PRs need to be updated as well. I'll fix that. Furthermore, in Menu, I used destructuring to extract isRtl as in TreeView which resulted in className missing from proptypes. I'll fix this as well.

What I understand from migrating TreeView is that it is not recommended to destructure the output of useThemeProps at the moment, because this results in incorrectly generated proptypes, right?

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Good to go :)

@mnajdova
Copy link
Member

Based on your reviews, I reviewed my other migration PRs. I see that index.d.ts of those PRs need to be updated as well. I'll fix that. Furthermore, in Menu, I used destructuring to extract isRtl as in TreeView which resulted in className missing from proptypes. I'll fix this as well.

Great! We may have missed that.

What I understand from migrating TreeView is that it is not recommended to destructure the output of useThemeProps at the moment, because this results in incorrectly generated proptypes, right?

Correct

@mnajdova mnajdova merged commit 13d5d4f into mui:next Apr 10, 2021
@tomasznguyen tomasznguyen mentioned this pull request Apr 10, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: tree view Changes related to the tree view. This includes TreeView, TreeItem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants