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 #26480

Closed
wants to merge 17 commits into from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented May 28, 2021

closes #13597
closes #26442

Another approach from #26446, please review ListItemButton.js

Summary

  • ListItemButton always render 2 DOM <li class="root"><div class="button"></li>

    <List>
      <ListItemButton ButtonBaseProps={{ component: 'a', href: '#id' }}>
      </ListItemButton>
    </List>
    
    // result
    <ul>
      <li> {/* ListItemButton-root */}
        <a href="#id"> {/* ListItemButton-button */}
        </a>
      </li>
    </ul>
    // nav
    <List component="nav">
     <ListItemButton component="div" ButtonBaseProps={{ component: 'a', href: '#id' }}>
     </ListItemButton>
    </List>
    
    // result
    <nav>
     <div> {/* ListItemButton-root */}
       <a href="#id"> {/* ListItemButton-button */}
       </a>
     </div>
    </nav>
  • accept secondaryAction as prop (wrapped with ListItemSecondaryAction) instead of children because it must be the same level as button (not inside).

    <ListItemButton
      secondaryAction={<IconButton><Checkbox /></IconButton>}
    >
      <ListItemText>Simple</ListItemText>
    </ListItemButton>
    
    // result
    <ul>
      <li class="root"> {/* ListItemButton-root */}
        <a class="button"> {/* ListItemButton-button */}
          <div>Simple</div>
        </a>
        <div class="secondaryAction"></div>
      </li>
    </ul>

    If this api sounds good to the team, will add to ListItem so that it does not need to iterate children to find secondary action

  • I have followed (at least) the PR section of the contributing guide.

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

mui-pr-bot commented May 28, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against fb1179a

@siriwatknp siriwatknp marked this pull request as ready for review May 31, 2021 02:32
@siriwatknp
Copy link
Member Author

@oliviertassinari @eps1lon please review the API, is it looks good to you?

I have one hesitation about naming

// option 1
<List>
  <ListItemButton ButtonBaseProps={{ component: 'a', href: '#id' }}>
  </ListItemButton>
</List>

// option 2
<List>
  <ListItemButton ButtonProps={{ component: 'a', href: '#id' }}>
  </ListItemButton>
</List>

I feel that option 2 might mislead user that the Button component is used, so I go with option 1 in this PR.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 31, 2021

I have one hesitation about naming

@siriwatknp With #21453, the consistent API would be:

<List>
  <ListItemButton componentsProps={{ button: { component: 'a', href: '#id' } }}>
  </ListItemButton>
</List>

IMHO, it's not great. The direction taken in #26446 looks better from a DX perspective.

@siriwatknp siriwatknp marked this pull request as draft June 3, 2021 03:17
@siriwatknp
Copy link
Member Author

decide to go with #26446 approach.

@siriwatknp siriwatknp closed this Jun 4, 2021
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFC] ListItem improvement [List] Improve ListItemSecondaryAction handling
3 participants