[WIP] create PopupState component

[jedwards1211]

Closes #12325

[oliviertassinari]

Most of the libraries I have seen seems to abandon the render property. What about not supporting it? So we save people asking themselves the question, "What's better between a and b?". Answering the question only provides a marginal gain.

[jedwards1211]

I sure wouldn't mind removing it, I only put it in there because some libs like react-powerplug seem to include it to appease people who think passing a child function is some kind of blasphemy

[oliviertassinari]

Well, I'm happy about being as opinionated as React.createContext API.

[jedwards1211]

Oh that's true. I'm glad the React folks decided to do that, because there were some really dogmatic people complaining that children should always be a React.Node.

It's about the same as how in all of the Angular vs. React vs. Vue type videos I've seen, people said they didn't like React because something about putting HTML in JavaScript didn't feel right to them. They didn't have any more substantive criticism than that.

[oliviertassinari]

@jedwards1211 How do you want to move forward? I haven't given a close look at the pull request yet but it looks solid. I see two options:
Option 1. I continue the effort with the variant thing. do some tweak where I seem some potential for improvement, let you have a final look.
Option 2. I do a deep review of the pull request, comment on all the points I see missing, try to provide guidance on how I see the variant come to life.
Let me know.

[jedwards1211]

@oliviertassinari I mainly just need to read about ARIA to see if we're missing any other props. Do you know of anything off the top of your head?

[jedwards1211]

@oliviertassinari I went ahead and added a variant property.

I also injected a few more useful props to the child function:

• toggle
• setOpen
• bindToggle

I haven't figured out if any more ARIA properties are needed yet. If you know of any well-organized ARIA guides for popups, let me know.

[jedwards1211]

The CI failed in the "Should not have any git not staged" section...I'm not sure what that's talking about, I would never expect there to be unstaged files in a CI environment.

[jedwards1211]

Also how can I prevent the generated API docs from saying "all other properties will be spread to the root element (native element)"? In this case there is no root element, and all other properties are simply ignored.

[oliviertassinari]

If you know of any well-organized ARIA guides for popups

@jedwards1211 Regarding the ARIA, I think that we can replicate what's documented on each variant documentation pages.

The CI failed in the

The markdown is automatically generated. The CI checks that it's up-to-date. You can run yarn docs:api.

In this case there is no root element, and all other properties are simply ignored.

We have the same problem with some other components. I think that you can add the exactProp proptype helper so people realize it at runtime. As for updating the description, well, it's a different story. We can ignore it for this pull request as it's a broader issue.

[jedwards1211]

Ah, maybe a better name for that CI section would be "All API docs should be up-to-date"?

I noticed in one of the menu demos, the trigger component uses aria-controls="lock-menu" instead of aria-owns when the menu is open. Seems like I should add aria-controls to bindTrigger and bindToggle?

I also noticed that the Popover demo doesn't use any relevant aria properties.

[jedwards1211]

@oliviertassinari don't merge this yet, I have a better idea about how to support a broader range of use cases like hover interaction. Instead of injecting bindTrigger/bindToggle, I would export helper functions as follows (note also that we don't need the variant prop). How should I generate docs for these exported functions?

export function bindTrigger({ isOpen, open, popupId }) {
  return {
    'aria-owns': isOpen ? popupId : null,
    'aria-haspopup': true,
    onClick: open,
  };
}

export function bindToggle({ isOpen, toggle, popupId }) {
  return {
    'aria-owns': isOpen ? popupId : null,
    'aria-haspopup': true,
    onClick: toggle,
  };
}

export function bindHover({ isOpen, open, close, popupId }) {
  return {
    'aria-owns': isOpen ? popupId : null,
    'aria-haspopup': true,
    onMouseEnter: open,
    onMouseLeave: close,
  };
}

export function bindPopover({ isOpen, anchorEl, close, popupId }) {
  return {
    id: popupId,
    anchorEl,
    open: isOpen,
    onClose: close,
  };
}

export const bindMenu = bindPopover;

export function bindPopper({ isOpen, anchorEl, popupId }) {
  return {
    id: popupId,
    anchorEl,
    open: isOpen,
  };
}

Then usage looks like this:

import React from 'react';
import PropTypes from 'prop-types';
import { withStyles } from '@material-ui/core/styles';
import Typography from '@material-ui/core/Typography';
import Popover from '@material-ui/core/Popover';
import PopupState, { bindHover, bindPopover } from '@material-ui/core/PopupState';

const styles = theme => ({
  popover: {
    pointerEvents: 'none',
  },
  paper: {
    padding: theme.spacing.unit,
  },
});

const PopoverPopupState = ({ classes }) => (
  <PopupState popupId="demoPopover">
    {(popupState) => (
      <div>
        <Typography {...bindHover(popupState)}>
          Open Popover
        </Typography>
        <Popover
          {...bindPopover(popupState)}
          className={classes.popover}
          classes={{
            paper: classes.paper,
          }}
          anchorOrigin={{
            vertical: 'bottom',
            horizontal: 'center',
          }}
          transformOrigin={{
            vertical: 'top',
            horizontal: 'center',
          }}
        >
          <Typography className={classes.typography}>The content of the Popover.</Typography>
        </Popover>
      </div>
    )}
  </PopupState>
);

PopoverPopupState.propTypes = {
  classes: PropTypes.object.isRequired,
};

export default withStyles(styles)(PopoverPopupState);

[jedwards1211]

Not cool, eslint-plugin-react:

// Handler function for onClick prop key must begin with 'handle' (react/jsx-handler-names)
<MenuItem onClick={popupState.close}>Test</MenuItem>

[jedwards1211]

I'm going to go ahead and close this in favor of the new API design I made. If you guys disagree for whatever reason let me know. I see the following advantages to the new design:

• Supports hover as well as click to open and click to toggle
• Easier to type with TS or Flow
• More tree-shakeable (unused bindX functions can be tree-shaken away)