Material UI menu drop down component
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) =&gt; {
  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 =&gt; { just to return something moments later seems like wasted code.


{children.map((child, i) =&gt; {
  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 =&gt; 
  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 () =&gt; {
    fn();
    this.handleMenuClose();
  };
else {
 return this.handleMenuClose;
}