Skip to content

[List] extract button from ListItem to ListItemButton#26480

Closed
siriwatknp wants to merge 17 commits intomui:nextfrom
siriwatknp:feat/list-item-button-flat
Closed

[List] extract button from ListItem to ListItemButton#26480
siriwatknp wants to merge 17 commits intomui:nextfrom
siriwatknp:feat/list-item-button-flat

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 scope: list Changes related to the list 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

scope: list Changes related to the list

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[RFC] ListItem improvement [List] Improve ListItemSecondaryAction handling

3 participants