Refactored SplitButton (breaking)

[kraenhansen]

✍️ Proposed changes

As per my suggestion in #2601, future regressions could be avoided if the types derived from MenuProps intended for pass-through is allow-listed via a Pick instead of deny-listed via Omit.

This change is breaking, since it's removing a lot of props from the SplitButton which were never actually passed through to the Menu.

This also optimize the internals by passing darkMode directly to child components instead of wrapping them in a new LeafyGreenProvider - this is not breaking, just a "drive-by" optimization.

NOTE: If #2601 merge before this PR, it needs to be updated to add renderDarkMenu in the allow-list.

✅ 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

• Add the @leafygreen-ui/split-button to a project
• Observe how the onEnter or renderDarkMenu props are available on the SplitButton.
• Apply this patch.
• Observe how the onEnter or renderDarkMenu props are no longer available on the SplitButton.

[changeset-bot]

🦋 Changeset detected

Latest commit: 20a5089

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

[shaneeza]

We still want to use the darkMode value returned from useDarkMode because it reads and returns the darkMode value from the parent LeafyGreenProvider. If darkMode is passed into the component, it will take precedence over the LeafyGreenProvider darkMode value.

[kraenhansen]

If darkMode is passed into the component, it will take precedence over the LeafyGreenProvider darkMode value.

That's the exact the behaviour I'd expect though?

I expect the Button and Menu (rendered internally in SplitButton) to call useDarkMode by themselves to apply the right theme from the provider iff a darkMode prop isn't passed to the SplitButton.

[shaneeza]

Thank you for pointing this out! You're absolutely correct.

However, we’ll stick with the current implementation, as we intentionally pass darkMode directly to the LeafyGreenProvider. This approach is deliberate and aligns with how we typically handle darkMode across other LeafyGreen components.

[kraenhansen]

I do see this pattern being used a lot of places and being documented here: https://github.com/mongodb/leafygreen-ui/blob/main/STYLEGUIDE.md#passing-darkmode-to-children

I'll go ahead and revert that change 👍

I do however think this decision should be revisited, wrapping components in multiple layers of these context providers isn't "free" and they all contribute to a messier component graph as well as potential rendering bottlenecks - especially when the context updates.

[kraenhansen]

I've drafted this again, since there are more changes that I'd like to include: It isn't possible to pass menu separators through the menuItems prop.

[shaneeza]

I think this makes sense, this way we are explicit with what props menu accepts

[shaneeza]

Thanks for the PR! The type updates make sense. However, we wouldn't want to remove the LeafyGreenProvider since passing darkMode to the LeafyGreenProvider is an explicit pattern we've adopted in other LG components.

[kraenhansen]

I've created one more PR with suggestioned breaking changes to the SplitButton API: #2604 - if those are accepted it would be ideal if they get released together, to avoid two major version bumps right after each other.