Refactored SplitButton (breaking - more)

[kraenhansen]

I'm working on a feature in Compass where I'd like to pass arbitrary elements to be rendered in the SplitButton menu.
I want to render a menu containing:

• MenuSeparator elements.
• MenuItem elements with controls that shouldn't cause the menu to close when clicked.
• Wrapped MenuItem elements to help me abstract a prop which needs to be passed to multiple props on the MenuItem.

The current implementation gets items through an menuItems prop (an array of React.ReactElement elements) - which has multiple drawbacks:

• Any element which is not a MenuItem is skipped when rendered, meaning it can't render separators, sub-menus or any components wrapping MenuItem - making it much less expressive with no apparent benefit.
• All the elements need to have a key prop provided.
• To my knowledge, this is not a pattern repeated elsewhere in the library.

✍️ Proposed changes

1. The types of the SplitButton currently declare taking children (from the ButtonProps) but these are never used internally. I suggest that these are instead passed into the Menu. This will make the menuItems prop obsolete.
2. To make the behavior of clicking a MenuItem configurable, I suggest adding an onItemClick prop taking a callback which will be called (just like onChange is called now) but also passed a second options object with a reference to the React element being clicked (to allow inspection of props) as well as a close function, which can be called to close the menu.
3. To avoid breaking the existing behavior of the menu closing when an item is clicked, I suggest a "default" implementation of the onItemClick used when no function is explicitly passed. Since the signature and semantics of this new onItemClick prop is so similar to onChange, I suggest removing the latter. The main caveat with this is that someone renaming their onChange callback to onItemClick now also has to explicitly call the close function in the callback.

Before

<SplitButton
  onChange={(event) => {
    console.log(event.currentTarget, "was selected in the menu");
  }}
  menuItems={[
    <MenuItem key="0">Menu Item</MenuItem>,
    <MenuItem key="1" disabled>
      Disabled Menu Item
    </MenuItem>,
    <MenuItem key="2" description="I am also a description">
      Menu Item With Description
    </MenuItem>,
  ]}
/>

After

<SplitButton
  onItemClick={(event, { close }) => {
    console.log(event.currentTarget, "was selected in the menu");
    close();
  }}
>
    <MenuItem>Menu Item</MenuItem>
    <MenuItem disabled>
      Disabled Menu Item
    </MenuItem>
    <MenuItem description="I am also a description">
      Menu Item With Description
    </MenuItem>
</SplitButton>

NOTE: This builds upon #2602 and needs to be rebased once that gets merged. Once rebased, I'll update the changeset description, including examples of how to migrate to the two new props (menuItems → children and onChange → onItemClick).

✅ Checklist

For bug fixes, new features & breaking changes

• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• I have run yarn changeset and documented my changes

For new components

• I have added my new package to the global tsconfig
• I have added my new package to the Table of Contents on the global README
• I have verified the Live Example looks as intended on the design website.

🧪 How to test changes

[changeset-bot]

🦋 Changeset detected

Latest commit: c0ff482

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@leafygreen-ui/split-button	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[TheSonOfThomp]

I believe we decided on the menuItems API to avoid confusion on whether the children should be the menu items or the button label

[kraenhansen]

avoid confusion on whether the children should be the menu items or the button label

That is easily disambiguated through a comment on the prop? Right now the component takes children and just throws them away? I feel that's more confusing 🤷 and much less expressive.

[kraenhansen]

I've updated the description above with more context on the issues I see with of the current menuItems prop.