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

[List] extract button from ListItem to ListItemButton #26446

Merged
merged 44 commits into from
Jun 7, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented May 25, 2021

the main changes are ListItem.js and ListItemButton.js, the rests are demos update.

ListItem

  • add secondaryAction prop to stop checking children
  • add disablePadding prop (similar to List) to use with ListItemButton
  • deprecate button related props

ListItemButton

  • support dense, alignItems, divider and disableGutters

docs

  • fix demos to use ListItemButton
  • add CustomizedList demo to test DX of the new API

image

closes #13597
closes #26442
closes #26469
closes #14971 (deprecate button prop)

Preview: https://deploy-preview-26446--material-ui.netlify.app/components/lists/

@siriwatknp siriwatknp added the component: list This is the name of the generic UI component, not the React module! label May 25, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented May 25, 2021

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.10% , gzip: +0.09%

Generated by 🚫 dangerJS against 0195818

@siriwatknp siriwatknp marked this pull request as ready for review May 26, 2021 05:32
docs/src/pages/components/lists/lists.md Outdated Show resolved Hide resolved
docs/src/pages/components/lists/lists.md Outdated Show resolved Hide resolved
packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
packages/material-ui/src/ListItemButton/ListItemButton.js Outdated Show resolved Hide resolved
@@ -33,25 +33,25 @@ export default function CheckboxList() {
<ListItem
key={value}
role={undefined}
Copy link
Member

@oliviertassinari oliviertassinari Jun 6, 2021

Choose a reason for hiding this comment

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

siriwatknp and others added 3 commits June 7, 2021 08:59
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@@ -304,12 +333,14 @@ ListItem.propTypes /* remove-proptypes */ = {
* If `true`, the list item is focused during the first mount.
* Focus will also be triggered if the value changes from false to true.
* @default false
* @deprecated will be removed in v6, checkout `ListItemButton` instead
Copy link
Member

Choose a reason for hiding this comment

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

Let's not mention any timeline for planned removals. They just become confusing if something changes and don't provide any value now. Mentioning the alternative should be enough.

Copy link
Member

@oliviertassinari oliviertassinari Jun 7, 2021

Choose a reason for hiding this comment

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

There is no visual feedback in https://deploy-preview-26446--material-ui.netlify.app/api/list-item/#props that it's deprecated.
Should we have a deprecatedPropType() as we make them deprecated in TypeScript?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

packages/material-ui/src/ListItem/ListItem.js Outdated Show resolved Hide resolved
docs/src/pages/components/lists/CustomizedList.js Outdated Show resolved Hide resolved
@eps1lon eps1lon assigned eps1lon and unassigned eps1lon Jun 7, 2021
@eps1lon
Copy link
Member

eps1lon commented Jun 7, 2021

For documentation we would need to use deprecatedPropType but then testing becomes non-trivial since we throw on unexpected console.error. However, we only receive these errors on the first test run and not in watchmode. So we can't easily integrate them with toErrorDev since React 17 no longer respects PropTypes.resetWarningCache and mocha doesn't have an API to reset modules.

We can do deprecation in a separate release though.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2021

Looking at the first demo in https://deploy-preview-26446--material-ui.netlify.app/components/lists/#simple-list, it looks like we are not following the canonical DOM structure:

nav > ul > li > button.

Should we update the demo?

@siriwatknp
Copy link
Member Author

Looking at the first demo in https://deploy-preview-26446--material-ui.netlify.app/components/lists/#simple-list, it looks like we are not following the canonical DOM structure:

nav > ul > li > button.

Should we update the demo?

Sure, will create a PR.

@siriwatknp siriwatknp deleted the feat/list-item-improvement branch June 13, 2021 10:29
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 13, 2021

For context, I found this working on #26711. It took me a while to find the List example I could copy with the new DOM structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module! deprecation New deprecation message
Projects
None yet
6 participants