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

Dropdown component #9893

Open
1 task done
turnerhayes opened this issue Jan 15, 2018 · 46 comments
Open
1 task done

Dropdown component #9893

turnerhayes opened this issue Jan 15, 2018 · 46 comments
Labels
component: menu This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@turnerhayes
Copy link

turnerhayes commented Jan 15, 2018

  • I have searched the issues of this repository and believe that this is not a duplicate.

Stemming from discussion in this bug, it would be great to have a "Dropdown" component. I see it as a sort of non-modal Popover--it closes in response to clicks outside, but doesn't swallow those clicks, so that you don't have to click twice to activate another interactive element on the page (once to dismiss the open Popover, then again to activate the other element).

My specific use case is that I have a pair of icons next to each other in an AppBar, that each open a small dropdown menu. I would like users to be able to switch between menus (or go to an input/whatever else on the page) without having to click twice.

For reference, I am converting a project of mine from reactstrap to Material UI, and previously I was using reactstrap's Dropdown component, which behaves how I would like this to behave. @oliviertassinari linked me to https://material.io/guidelines/components/buttons.html#buttons-dropdown-buttons as the Material UI spec that seems to specify this case.

@oliviertassinari oliviertassinari added the new feature New feature or request label Jan 15, 2018
@justinryder
Copy link

I would love this feature as well. I wrote my own Dropdown component that wraps Menu and I've tried all manner of tweaks to the Menu props (and Popover props and Modal props and Paper props) to try and get the backdrop to stop eating clicks, but still close when clicking outside. Would be awesome for there to be 1st class support for this use case in MUI.

@oliviertassinari oliviertassinari added this to the post v1 milestone Feb 11, 2018
@zebra-ok
Copy link

I'm also looking for a Dropdown menu component, would you mind sharing your code? @justinryder

@oliviertassinari
Copy link
Member

Update: we have a new Popper component. It's a building block we use to create a Dropdown component.

previously I was using reactstrap's Dropdown component

@turnerhayes I'm wondering if we shouldn't allow people to switch between the Popper and Popover component when using the Menu component. They use different tradeoffs, Popover is better suited for mobile when Popper is better suited for desktop.

I would like users to be able to switch between menus (or go to an input/whatever else on the page) without having to click twice.

We have a hideBackdrop property. We could update the Modal CSS to make the click go through the Modal.

@oliviertassinari oliviertassinari added priority: important This change can make a difference component: menu This is the name of the generic UI component, not the React module! labels Mar 13, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 21, 2019

This demo demonstrates how a dropdown like experience can be built: https://material-ui.com/demos/menus/#menulist-composition. It composes the ClickAwayListener, Popper and Grow components.

Capture d’écran 2019-03-21 à 09 49 18

Also, if you don't care about the scroll locking you can use the Menu and change the display position like here: https://next.material-ui.com/guides/interoperability/#portals.

Capture d’écran 2019-03-21 à 09 48 54

@ryancogswell
Copy link
Contributor

ryancogswell commented May 6, 2019

@oliviertassinari I plan on tackling dropdowns next (while continuing to also help gradually move #15597 forward). It will be a while before I get to the point of a pull request. I'll need to spend some time digging into the details of how the positioning works for Popper and Popover. I'll probably start doing some proof-of-concept work in sandboxes over the next couple weeks. I'll post links here to any sandboxes showing anything of interest that I want feedback on.

My intent is to support this via a dropdown variant for Menu and to support using the dropdown Menu variant with Select as well (either via MenuProps or possibly some new prop directly on Select?) which would result in an "exposed dropdown".

As far as related issues, I see #11243, #10804, and #15055. Are there any others you know about that I should be looking at while working on this?

For my own reference, this is a modified version of the MenuList composition demo as a starting point for exploration: https://codesandbox.io/s/x739zr9684

Material Design references:
https://material.io/design/components/menus.html#dropdown-menu
https://material.io/design/components/menus.html#exposed-dropdown-menu

@oliviertassinari
Copy link
Member

oliviertassinari commented May 6, 2019

@ryancogswell This is a weak spot of the library. I know that the documentation could be improved to better illustrate how to change the menu position, same for the select component. But we have more important issues with the implementation. I think that we should first agree with what's wrong. The issues I'm aware of:

  • There are cases where you don't want the menu to be using a Modal internally. You might want to allow the scroll or to click on a different element (rather than displaying a backdrop). At some point, I was interested in having the Popper & Popover two interchangeable components.
  • When you get the focus on an element, you should be able to open the dropdown using the arrow keys.

@ryancogswell
Copy link
Contributor

ryancogswell commented May 6, 2019

There are cases where you don't want the menu to be using a Modal internally.

@oliviertassinari My thought was that the dropdown variant would always use Popper and allow clicking immediately on other elements, though once I do the work to support that, it would be fairly trivial to have a separate property control that. I think it would make sense for the dropdown variant to default to using Popper while the other variants would continue to default to Popover.

When you get the focus on an element, you should be able to open the dropdown using the arrow keys.

For Select, this already works. Would you want this outside of Select? I don't think a user would generally expect an arrow key to do anything to a button.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 7, 2019

@ryancogswell I agree, a Dropdown menu without Popper is something else.
Let's take Gmail as an example, people can open this dropdown menu using the Down Arrow:

Capture d’écran 2019-05-07 à 12 39 58

Once we have wrapped our head around the use cases we want to support, we can think of the best API.

@ryancogswell
Copy link
Contributor

ryancogswell commented May 7, 2019

@oliviertassinari Here is a relevant resource that provide further support for that keyboard approach: https://www.w3.org/WAI/ARIA/apg/patterns/menubutton/.

down arrow and up arrow are listed as optional controls for menu buttons that open the menu and put focus on the first or last item respectively (though playing around with some different apps, I don't see this differentiation -- both arrow keys just open and select the first item).

This means taking more control of the menu button which should allow us to improve the DX considerably for typical menu cases. Our Simple Menu example could become something like:

      <Menu id="simple-menu" menuButton={<Button>Open Menu</Button>}>
        <MenuItem>Profile</MenuItem>
        <MenuItem>My account</MenuItem>
        <MenuItem>Logout</MenuItem>
      </Menu>

with Material-UI handling all of the aria properties, open/close, and anchor element.

This paradigm could also be leveraged eventually for creating cascading menus:

      <Menu id="cascading-menu" menuButton={<Button>Pick a food</Button>}>
        <MenuItem>Pie</MenuItem>
        <SubMenu menuButton={<MenuItem>Fruit</MenuItem>}>
           <MenuItem>Apple</MenuItem>
           <MenuItem>Orange</MenuItem>
        </SubMenu>
        <MenuItem>Pizza</MenuItem>
      </Menu>

If you think this API approach (menuButton property) makes sense, I'll go ahead and tackle that as a separate effort first before continuing with the dropdown/Popper enhancement. This should probably include reworking SelectInput to leverage the menuButton property, otherwise a lot of the logic would be duplicated since SelectInput already does much of the work that this new approach would handle more broadly for menus.

Separate minor issue that would make sense to fix at the same time is the aria-haspopup value. Currently we are setting this to true which is equivalent to menu, but for Select I believe we should be setting it to listbox instead (it should match the role of the popup element).

https://www.w3.org/TR/wai-aria-1.1/#aria-haspopup

@oliviertassinari
Copy link
Member

oliviertassinari commented May 8, 2019

@ryancogswell I have looked at a couple of Menu dropdown components:

Out of this, I can observe that there is 2 popular APIs:

  • The children slot pattern x4, e.g.
<MenuDropdown>
  <MenuTrigger><Button>Oh snap</Button></MenuTrigger>
  <Menu>
    <MenuItem>Item n°1</MenuItem>
  </Menu>
</MenuDropdown> 
  • The prop slot pattern x5, e.g.
<MenuDropdown
  content={
    <Menu>
      <MenuItem>Item n°1</MenuItem>
    </Menu>
  }
>
  <Button>Oh snap</Button>  
</MenuDropdown> 

What we do in Material-UI is uncommon. I have already seen people asking for a better API. material-ui-popup-state is a crystallization of this frustration (by @jedwards1211).

But what should we do? I would probably avoid a hooks API as it might rerender too many elements and allow too many moving parts.

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 9, 2019

@ryancogswell how do you like the hooks API in material-ui-popup-state? I definitely intend it to be the ideal balance of convenience and flexibility. It has a cascading submenus example, all it lacks is keyboard handlers for opening and closing submenus, which I have been meaning to add at some point

@ryancogswell
Copy link
Contributor

how do you like the hooks API in material-ui-popup-state?

@jedwards1211 Tonight is my first time looking at material-ui-popup-state, so I'm still digesting the approaches and the alternatives. A solution built directly into the Material-UI components has some options that aren't really available in userland. I see a lot of useful ideas in material-ui-popup-state, but I think a solution directly in the library can be implemented more declaratively and with approaches that are more consistent with the rest of the library. The hooks API exposes a lot that I would prefer to keep as implementation details that are encapsulated more from users, so that we have more freedom to change those details as it evolves.

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 9, 2019

If someone needs to pass a custom component in the trigger or menu position in the API you're proposing, I guess it will have to make sure to pass through a bunch of props injected by the API?

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 9, 2019

I have always been very skeptical of the flexibility of the approaches Olivier outlined. Though maybe that's not necessary if the API you're proposing is only intended for use with specific, unwrapped Material UI components.

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 9, 2019

Also, relying on cloneElement to inject some props can cause headaches with TypeScript/Flow if any of those props are required, unfortunately. For that reason i have a strong bias against using cloneElement

@ryancogswell
Copy link
Contributor

Also, relying on cloneElement to inject some props can cause headaches with TypeScript/Flow if any of those props are required

I'll try to keep that in mind -- I don't use TypeScript myself, so those issues aren't on the top of mind. The open prop on Menu is the main case that might fall into this category without some care. One advantage of cloneElement is that we can look at the properties already on the element and decide whether the existing properties should win over what we might specify or if they should be wrapped (e.g. event handlers) with additional functionality added.

@jedwards1211
Copy link
Contributor

Yeah, merging event handlers is more of a hassle with the material-ui-popup-state approach

@jedwards1211
Copy link
Contributor

I think it would be fine to make open optional for the sake of preventing typescript/flow errors with the proposed API

@ryancogswell
Copy link
Contributor

@jedwards1211 Not sure if you are aware yet, but your cascading menus are broken in v4: https://codesandbox.io/s/yv7v8vv00j. The onMouseLeave is no longer working in the same way. I suspect it is due to changes to Modal, but I didn't fully troubleshoot it.

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 9, 2019

Not surprising, I haven't had time to try out v4 yet, and preventing Modal from blocking mouse events is really ugly, at least in v3. I should add a peerDependency if I haven't already though.

@ryancogswell
Copy link
Contributor

ryancogswell commented May 10, 2019

For the moment, I want to ignore the overall structure of the solution and just document the different aspects that the solution needs to control/impact. Below I've documented what I believe to be all the key aspects for common use cases. @oliviertassinari and @jedwards1211 -- let me know if you think I'm missing anything (or if I've included anything that you think we definitely don't need to include).

Popup Open Trigger Button receives:

  • onClick (controls popup open)
  • onKeyDown (controls popup open)
  • aria-owns (from popup id)
  • aria-haspopup (from popup role)
  • possibly onMouseEnter (controls popup open/close)
  • possibly onMouseLeave (controls popup open/close)

popup (Menu/Popover/Popper) receives:

  • open
  • onClose (controls popup close)
  • anchorEl (from trigger button ref)
  • possibly other positioning properties
  • possibly onMouseEnter (controls popup open/close)
  • possibly onMouseLeave (controls popup open/close)

MenuItem receives:

  • onClick (controls Menu close, needs to wrap user-specified onClick)

MenuItem doesn't apply to Popover/Popper, but they do need some way of signaling when the popup should be closed. The main options are click-away, Esc, or some sort of "close" button within the popup. A "close" button within the popup could be supported via a PopopCloseTrigger component which could be wrapped around a button and would take care of providing/wrapping the button's onClick. Basically, this PopopCloseTrigger would serve the same purpose for Popper and Popover that MenuItem provides for Menu.

I have further thoughts on possible alternatives for the structure of the controlling component(s). I'll try to find time to document the rest of my thoughts sometime over the weekend.

@ryancogswell
Copy link
Contributor

@jedwards1211 After some thought on context menus, The pattern I think I would want would look something like:

<List contextMenu={<MyMenu/>}>
   <ListItem contextData={{data: 'to be provided', whenTheMenu: 'is triggered'}}>Item 1</ListItem>
   <ListItem contextData={{differentData: 'to be provided', whenTheMenu: 'is triggered', forThis: 'item'}}>Item 2</ListItem>
</List>

But I'm not sure if context menus are a common enough use case to warrant something like this in the library. They would typically only be in screens intended for advanced users (not typical end users).

@ryancogswell
Copy link
Contributor

ryancogswell commented May 11, 2019

A more generic (not tied to List) version might look like:

<ContextMenuProvider menu={<MyMenu/>}>
   <List>
      <ContextData data={{data: 'to be provided', whenTheMenu: 'is triggered'}}><ListItem>Item 1</ListItem></ContextData>
      <ContextData data={{differentData: 'to be provided', whenTheMenu: 'is triggered', forThis: 'item'}}><ListItem>Item 2 <ContextMenuTrigger><MyIconButton title="Could have something like this to trigger the context menu via a button"/></ContextMenuTrigger></ListItem></ContextData>
   </List>
</ContextMenuProvider>

This would work for a broader number of use cases, so might have a better chance of eventually being in the library, but it also could be done fully in userland without looking any different than if it were in the library (I don't think it would require changes to existing components).

The menu provider probably needs to be more like the following in order to be able to leverage the context data for rendering the menu:

<ContextMenuProvider renderMenu={(contextData, positioningProps) => (<MyMenu {...positioningProps} contextData={contextData}/>)}>

@jedwards1211
Copy link
Contributor

As long as you guys don't get rid of the existing low-level API it's fine...this new API proposal would make simple, common cases super convenient, but wouldn't be able to help with some problems, so we'll need to be able to fall back on the existing API.

material-ui-popup-state is not quite as convenient for the simple common cases, but it's handy enough in a large variety if contexts. I like its position on the flexibility/convenience plane.

@ryancogswell
Copy link
Contributor

As long as you guys don't get rid of the existing low-level API

I picture doing this entirely as a non-breaking change that you would opt into via the button prop.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 12, 2019

I believe the reason n°1 behind the popularity of this API:

<MenuDropdown
  content={
    <Menu>
      <MenuItem>Item n°1</MenuItem>
    </Menu>
  }
>
  <Button>Oh snap</Button>  
</MenuDropdown>

is the fact that it's closer to the actual DOM output. You get the button as a child of the parent hierarchy and the content mounted in a portal.


<Popper button={<MyButton/>}>
  <MyContent/>
</Popper>

The Popper component was designed as a small alternative to react-popper. I think that it will be better if the component only focuses on the positioning relative to another element and do no handle the display coordination or the accessibility.

@jedwards1211
Copy link
Contributor

FWIW I think this whole proposed API can be made with userland wrapper components too, and I'm now tempted to do so :) but it would be a welcome addition to MUI 4 as well.

Also, I think MenuItem won't even need a special onClick handler...now that I think about it those events will propagate up to the Menu and we can just handle them there.

@ryancogswell
Copy link
Contributor

those events will propagate up to the Menu and we can just handle them there

@jedwards1211 The events will propagate up, but I don't think the Menu will know enough to know what to do. The user could have clicked on a divider, or a disabled item, or a button for opening a sub-menu. I think you need the context at the MenuItem to know whether or not the click should trigger a close.

I think this whole proposed API can be made with userland wrapper components

I think that is definitely true of the wrappers for Popper and Popover, however the trigger for this whole conversation was wanting to support a dropdown variant for Menu and to allow down arrow to control opening the Menu in addition to button click when focus is on the button. That is the key aspect that I want directly in Material-UI in the near future.

@ryancogswell
Copy link
Contributor

ryancogswell commented May 12, 2019

You get the button as a child of the parent hierarchy and the content mounted in a portal.

@oliviertassinari I do get this argument, and if we were starting from scratch I might be more inclined to go that route. The main counter-argument is that the important thing the user is trying to achieve and the part with more potential for structural complexity is the menu, so it feels awkward to have it be the prop and to use the children for the simple button. To me, the button is more of a minor implementation detail -- almost like specifying the expandIcon on ExpansionPanelSummary. Conceptually, the user is sticking a menu in that spot -- not a button; they just don't want it to take up much real estate or clutter the screen when it isn't in active use. Also, our Menu component is already something that you can define inline next to your button even though it isn't rendered directly in the hierarchy. Having the button as a prop is basically saying, "this is the thing that I want you to show as a placeholder for my menu when the whole thing isn't being shown".

How strongly do you feel about which direction we should go? I feel pretty strongly that my proposal (just talking about Menu right now, not the extension to Popper and Popover) is the DX that I would personally prefer (especially given how cleanly it extends to support cascading menus), but I'm not sure how best to gauge how a wider audience would feel.

To summarize, I think we're trying to choose between:

a.

<MenuDropdown
  content={
    <Menu>
      <MenuItem>Item 1</MenuItem>
      <MenuItem>Item 2</MenuItem>
      <MenuItem>Item 3</MenuItem>
    </Menu>
  }
>
  <Button>Open Menu</Button>  
</MenuDropdown>

vs.

b.

<Menu button={<Button>Open Menu</Button>} variant="dropdown">
  <MenuItem>Item 1</MenuItem>
  <MenuItem>Item 2</MenuItem>
  <MenuItem>Item 3</MenuItem>
</Menu>

And for cascading menus (eventually):

a.

<MenuDropdown
  content={
    <Menu>
      <MenuItem>Item 1</MenuItem>
      <Menu button={<MenuItem>Item 2</MenuItem>}>
        <MenuItem>Item 2a</MenuItem>
        <MenuItem>Item 2b</MenuItem>
        <MenuItem>Item 2c</MenuItem>
      </Menu>
      <MenuItem>Item 3</MenuItem>
    </Menu>
  }
>
  <Button>Open Menu</Button>  
</MenuDropdown>

vs.

b.

<Menu button={<Button>Open Menu</Button>} variant="dropdown">
  <MenuItem>Item 1</MenuItem>
  <Menu button={<MenuItem>Item 2</MenuItem>}>
    <MenuItem>Item 2a</MenuItem>
    <MenuItem>Item 2b</MenuItem>
    <MenuItem>Item 2c</MenuItem>
  </Menu>
  <MenuItem>Item 3</MenuItem>
</Menu>

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 12, 2019

A userland Menu wrapper component/DropdownMenu wrapper component in this lib could inject the arrow key listener into the button props right?
I bring this up because maybe we could keep those extra bytes out of Menu for people who don't use the dropdown variant.

@ryancogswell
Copy link
Contributor

The main thing that can't be in userland is to have Menu use Popper internally for dropdowns instead of Popover so that it isn't modal, so we want to go ahead and improve the overall DX at the same time so that for typical use cases, it all just works.

@jedwards1211
Copy link
Contributor

I see, yeah using a popper would probably avoid some of the tricky mouse blocking issues i was talking about with popover

@oliviertassinari
Copy link
Member

To me, the button is more of a minor implementation detail

@ryancogswell Thanks for highlighting it. I would be happy with both approaches. I have found another library to benchmark (nested menus): https://reakit.io/docs/menu/. My main concern would be about considering what others are doing. Option b looks better for nested menus.
cc @mui-org/core-contributors.

@ryancogswell
Copy link
Contributor

I would be happy with both approaches.

@oliviertassinari Sounds good. I think I have enough definition to move forward. I think we can work through any other details during code review. I think even this aspect of the main API structure would be a fairly easy aspect to change during code review if we decide on a different direction.

@eps1lon
Copy link
Member

eps1lon commented May 16, 2019

Option b looks fine. Would be nice to have a comprehensive comparison with other popular APIs though.

@oliviertassinari
Copy link
Member

@eps1lon I have done a short comparison in #9893 (comment).

@ryancogswell
Copy link
Contributor

ryancogswell commented May 16, 2019

@eps1lon One thing to note when looking at the comparisons is just the difference in our starting point. For example if we look at one example using option "a":

https://blueprintjs.com/docs/#core/components/menu.dropdowns

<Popover content={<Menu>...</Menu>} position={Position.RIGHT_TOP}>
    <Button icon="share" text="Open in..." />
</Popover>

This Blueprint example includes this note:

The Menu component by itself simply renders a list of items.

So the Blueprint Menu is more similar to our MenuList. It truly is just the menu content. A lot of what the Blueprint Popover does is already being done by our Menu with the exception of handling the button. This option "a" approach gets a bit muddled for us as far as naming and responsibilities if we have some new component that has a Menu passed as a property due to the overlap in responsibilities.

@DeylEnergy

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants