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

[material-ui][ListItem] Remove props which have been deprecated since 2021 #41296

Closed
DiegoAndai opened this issue Feb 27, 2024 · 11 comments
Closed
Assignees
Labels
component: list This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@DiegoAndai
Copy link
Member

DiegoAndai commented Feb 27, 2024

Remove props deprecated since May 2021 (#26446):

  • autoFocus
  • button
  • disabled
  • selected

The ContainerComponent and ContainerProps will be handled in #41281

Search keywords: deprecated listitem

@DiegoAndai DiegoAndai added this to the Material UI: v6 milestone Feb 27, 2024
@DiegoAndai DiegoAndai moved this to Selected in Material UI Feb 27, 2024
@github-actions github-actions bot added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 27, 2024
@DiegoAndai DiegoAndai added component: list This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 27, 2024
@thathva
Copy link
Contributor

thathva commented Mar 1, 2024

Is this open? Can I assign myself to this?
I have taken this up since no one is assigned to it and it's ready to take.

@thathva
Copy link
Contributor

thathva commented Mar 4, 2024

Correct me if I am wrong, the changes would be:

  • To the ListItem API in the material-ui package by removing the listed props and its dependencies.
  • Modify the documentation in material-ui and remove the props from the list of props.

@DiegoAndai
Copy link
Member Author

Hey @thathva, sorry for the late reply. Yes, feel free to work on this.

The changes would be:

  • Remove the props from the ListItem component
  • Remove the props from ListItem.d.ts
  • Run pnpm proptypes and pnpm docs:api which should update the documentation

We'll make this change in v6 alpha, so the PR for this change should point to the next branch. That branch doesn't exist now but should be created within the next week.

Feel free to let me know if you need any help.

@thathva
Copy link
Contributor

thathva commented Mar 6, 2024

Hey @DiegoAndai! Thank you for the response!
I was able to go ahead and do the changes as you mentioned and the API docs looks great so far. I just had couple of follow up questions:

  1. Would I need to update test cases as well? I see some test cases in ListItem.test.js using the deleted props.
  2. In the same ListItem.js file, I see some other references to the props that are to be deleted. Do I comment these out or remove it totally? For example, for autoFocus there is this bit of code:
 useEnhancedEffect(() => {
    if (autoFocus) {
      if (listItemRef.current) {
        listItemRef.current.focus();
      } else if (process.env.NODE_ENV !== 'production') {
        console.error(
          'MUI: Unable to set focus to a ListItem whose component has not been rendered.',
        );
      }
    }
  }, [autoFocus]);
  1. Should I remove the props for both the Props and CSS Classes or would just Props do?

@DiegoAndai
Copy link
Member Author

Hey @thathva!

  1. We should remove the tests that test the deleted prop. If a test is not testing the deleted prop but relies on it, we should refactor it.
  2. We should remove them totally.
  3. You mean the MuiListItem-button class? We should remove it.

@thathva
Copy link
Contributor

thathva commented Mar 11, 2024

Hey @DiegoAndai
I think I am done with all the changes. I have removed the said props from both the Props and CSS classes in the material-ui packages. The test cases were deleted as all were directly testing the prop. Can you let me know when I can raise a pull request and against which branch?
Thanks!

@DiegoAndai
Copy link
Member Author

Hey! Here's the contributing guide that explains how to open pull requests: https://github.com/mui/material-ui/blob/master/CONTRIBUTING.md#sending-a-pull-request

We should open it against the next branch, which will be created next week. So we'll have to hold on until that.

@thathva
Copy link
Contributor

thathva commented Mar 19, 2024

Hey!
Please let me know when the next branch is created, so that I can raise a pull request against that. Thanks!

@DiegoAndai
Copy link
Member Author

Hey! It's now open 🎉
You can open the pull request.

@thathva
Copy link
Contributor

thathva commented Mar 20, 2024

Hey @DiegoAndai
I have raised a pull request against the next branch. Although it passes all the workflow actions, the Check if PR has label is failing, even though I seem to have followed the format 🤔.
Let me know if I have to make any modifications to the code!

@DiegoAndai DiegoAndai moved this from Selected to Backlog in Material UI Apr 22, 2024
@DiegoAndai DiegoAndai moved this from Backlog to In progress in Material UI May 3, 2024
@DiegoAndai DiegoAndai self-assigned this May 3, 2024
@DiegoAndai DiegoAndai moved this from In progress to Selected in Material UI May 24, 2024
@mnajdova mnajdova assigned mnajdova and unassigned DiegoAndai Jul 31, 2024
@DiegoAndai
Copy link
Member Author

Completed in #41566

@github-project-automation github-project-automation bot moved this from Selected to Done in Material UI Aug 8, 2024
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! package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
Status: Done
Development

No branches or pull requests

3 participants