Material UI menu drop down component

Question

The reason I created this is because the Material UI way of doing a simple 'menu that opens below the button' is convoluted as hell (see the 'toggle menu grow' example, and check the code out. I've basically copied the code for that, and put it in one nice wrapper.

Codepen

Here's the usage of the component:

<NavDropdownMenu
  buttonProps={{
    variant: "contained",
    color: "secondary"
  }}
  buttonChildren={"foo bar"}
>
  <MenuItem onClick={this.handleA}>Profile</MenuItem>
  <MenuItem onClick={this.handleB}>My account</MenuItem>
  <MenuItem>Logout</MenuItem>
</NavDropdownMenu> 

Notes:

• We can pass through whatever material props to the button.
• We can put whatever content we like inside the button. (e.g. icons, avatars).
• We put whatever we like as menu items, and bind our own onclick events to them.

Under the hood, in order to both trigger the close menu functionality, as well as allow the user to bind their own click events to the menu items, I have this code that uses React.cloneElement:

   <MenuList>
    {children.map((child, i) => {
      return React.cloneElement(child, {
        ...child.props,
        ...{
          onClick: this.generateOnClick(child.props.onClick),
          key: `${menuId}${i}`
        }
      });
    })}
  </MenuList>

And that function:

  generateOnClick = fn => {
    if (fn)
      return () => {
        fn();
        this.handleMenuClose();
      };
    else {
      return this.handleMenuClose;
    }
  };

Now, I haven't put in PropTypes; I have to confess I'm not in the habit of using them. And there's probably some more extensibility I could add - (e.g. configuring the placement of the menu).

But other than that, are there glaring issue with this code that make it a bad idea?

Answer 1

As a general sentiment, I can't spot any glaring issues with the code just the one section and a few minor things (you did say you basically copied it; I'd imagine the Material UI team have some idea what they're doing)

---

{children.map((child, i) => {
  return React.cloneElement(child, {
    ...child.props,
    ...{
      onClick: this.generateOnClick(child.props.onClick),
      key: `${menuId}${i}`
    }
  });
})}

children cannot always be defined as an array, if you pass a single list item it will be given as a single element which will not have map as a property. You should be using React.Children.map instead.

Slipping in a small code style comment here, doing => { just to return something moments later seems like wasted code.

{children.map((child, i) => {
  return React.cloneElement(child, {

When using React.cloneElement, you don't need to manually specify the child props as that's the implication of cloning.

return React.cloneElement(child, {
  ...child.props,

Creating a separate object inside another object just to spread is redundant.

...{
  onClick: this.generateOnClick(child.props.onClick),
  key: `${menuId}${i}`
}

To finish things off here, using the index of the child as part of the key is an anti-pattern. Any time that someone wants to change the children, or reorder the list, React will assume it has already memoized the render during the previous reconcile and give you the wrong render.

There's a number of resources and in-depth explanations if you Google something along the lines of React index as key. Generally when building reusable components, I leave the key generating relative to each implementation, but, that's your decision, however, I would say that some developers want to use their own keys and manually overriding them is blocking that.

key: `${menuId}${i}`

A better implementation would be:

{React.Children.map(children, child => 
  React.cloneElement(child, {
    onClick: this.generateOnClick(child.props.onClick)
  })
)}

Even so, you're cloning all of your children inside each render of the component, with all the information used in that process being loaded in during the initialisation phase.

Everytime you act on the menu, you're performing the same element cloning on all your children again. If I had complex children or even a large amount, this could easily slow the render process.

---

A few minor points:

• buttonChildren would better be named label or menuLabel
• Using propTypes is very beneficial, especially if you're trying to create a reusable component/library, not only to the developer in charge of implementation to know what to expect but you (or whoever) in charge of fixing potential bugs in the library.

• Usually the Popper component allows you to open and close a menu by clicking it... for whatever reason, you can't close the component by clicking it.

• Stick with consistent bracketing around your if/else

  if (fn)
    return () => {
      fn();
      this.handleMenuClose();
    };
  else {
   return this.handleMenuClose;
  }