mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

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

Open DiegoAndai opened 2 months ago

DiegoAndai commented 2 months ago

Remove props deprecated since May 2021 (https://github.com/mui/material-ui/pull/26446):

The ContainerComponent and ContainerProps will be handled in https://github.com/mui/material-ui/issues/41281

Search keywords: deprecated listitem

thathva commented 2 months ago

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 commented 2 months ago

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

DiegoAndai commented 2 months ago

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

The changes would be:

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 commented 2 months ago

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]);
  3. Should I remove the props for both the Props and CSS Classes or would just Props do?
DiegoAndai commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

DiegoAndai commented 2 months ago

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

thathva commented 1 month ago

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!