Material-ui: [Menu] Vertical anchoring of Popover

[bloomdido]

Problem description

If you set the vertical value of the anchorOrigin prop for the Popover component to anything other than 0 or center, you do not get the desired result. I believe this line is the culprit:

https://github.com/callemall/material-ui/blob/1a9b612c02cd606a3ae31c8a241505a2e8e67264/src/Popover/Popover.js#L306

Looks like the expressions of the ternary operator are reversed.

Steps to reproduce

Create a Popover or other component that uses a Popover (such as a Menu) and set a anchorOrigin: vertical value of something other than 'center' or 0.

<Popover anchorOrigin={{vertical: 'bottom'}} ... />

Versions

• Material-UI: v1.0.0-beta.6
• React: 15.6.1
• Browser: Any

[oliviertassinari]

What's the desired result? The behavior seems ok to me:

[bloomdido]

Is that from v1.0.0-beta?

[bloomdido]

That is what I'd expect. I'm not seeing that behavior from v1.0.0 however.

[oliviertassinari]

This demo is coming from #7927. The live documentation hasn't been updated yet.

[bloomdido]

I don't know what that demo is supposed to be showing, but the following code does not work with v1.0.0-beta.6:

import React, { Component } from 'react';
import Button from 'material-ui/Button';
import Menu, { MenuItem } from 'material-ui/Menu';

class SimpleMenu extends Component {
  state = {
    anchorEl: undefined,
    open: false,
  };

  handleClick = event => {
    this.setState({ open: true, anchorEl: event.currentTarget });
  };

  handleRequestClose = () => {
    this.setState({ open: false });
  };

  render() {
    return (
      <div>
        <Button
          aria-owns={this.state.open ? 'simple-menu' : null}
          aria-haspopup="true"
          onClick={this.handleClick}
        >
          Open Menu
        </Button>
        <Menu
          id="simple-menu"
          anchorEl={this.state.anchorEl}
          open={this.state.open}
          onRequestClose={this.handleRequestClose}
          anchorOrigin={{vertical: 'bottom', horizontal: 'right'}}
        >
          <MenuItem onClick={this.handleRequestClose}>Profile</MenuItem>
          <MenuItem onClick={this.handleRequestClose}>My account</MenuItem>
          <MenuItem onClick={this.handleRequestClose}>Logout</MenuItem>
        </Menu>
      </div>
    );
  }
}

export default SimpleMenu;

That code is copied _verbatim_ from the current v1 docs, with the addition of the anchorOrigin property. When I run it, the menu correctly drops down from the right side of the button, but not from the bottom.

[bloomdido]

Perhaps it is a bug with the Menu component instead, but when I look at the code for Menu, it passes the props on to Popover, and I am 99% sure the line of code I referred to above is the problem...

[oliviertassinari]

Could you provide a reproduction test case? That would help a lot 👷 .
You can use codesandbox.io for instance.

[oliviertassinari]

By bad, you have provided one.

[oliviertassinari]

The observed result is the expected one. However, I think that we should be adding a warning when the anchorOrigin.vertical is set at the same time of the getContentAnchorEl property. We can't have both at the same time.

[oliviertassinari]

Add getContentAnchorEl={null} and you should be good.

[bloomdido]

Is this going to work for the Menu component? Looking at the source, it appears that Menu sets the getContentAnchorEl prop of the Popover directly. It does not get passed through.

[oliviertassinari]

I haven't tried, but the source code makes me think that users can override the getContentAnchorEl property provided by the Menu to the Popover.

[bloomdido]

👍 You're right. I see now that if you set the getContentAnchorEl prop on Menu, it will override the one the Menu sets on Popover because of {...other}. Tested it out and it worka here. Probably should add something to the documentation for Menu.

Optionally, you could just set getContentAnchorEl to null from Menu if an anchorOrigin prop is passed to Menu with a value for vertical. It can be easily done from the getContentAnchorEl function already present in Menu.

[CorayThan]

I was really confused about why I was getting that error just from adding anchorOrigin to a Menu. It should really just ignore getContentAnchorEl if anchorOrigin is there, or at least the warning should instruct users to null out getContentAnchorEl.

[oliviertassinari]

@CorayThan You are right. We need to:

1. Add a description for getContentAnchorEl
2. Improve the warning error message to explain what's going on and with an actionable message.

[smeijer]

We need to:

How about

3) set getContentAnchorEl to null when the caller isn't doing anything with it, and just want to provide the `anchorOrigin to the menu.

[oliviertassinari]

@smeijer This sounds like a good idea, do you want to submit a pull request or opening a dedicated issue to describe the behavior?

[dy]

I'm sorry - just to clarify - why that issue was closed? 4.0.0 alpha7 does not seem to have getContentAnchorEl={null} documented.

[oliviertassinari]

@dy You need to follow the props cascading. getContentAnchorEl is documented on the Popover.

Any other properties supplied will be spread to the root element (Popover).

[oliviertassinari]

We will improve the demos in #10804.

[obendesmond]

thanks

[pbasista]

getContentAnchorEl is documented on the Popover.

That is correct, it is mentioned there. But if I am looking correctly, that page contains no explanation or warning about the necessity to override it to null when using anchorOrigin prop with a value for vertical.