Skip to content

Conversation

@geotrev
Copy link
Contributor

@geotrev geotrev commented Sep 10, 2024

Description

This PR addresses a small bug with menu button ref handling, in addition to improving ref handling between buttonProps and button in a Menu.

Detail

The main fix is reversing Menu's ref (from forwardRef) from merging with triggerRef when composing button props. Instead, the ref from buttonProps is merged.

Now a consumer could achieve something like this:

<Menu buttonProps={{ ref: buttonRef }}>
  ...
</Menu>

... and the ref would be maintained. Compared to what a consumer would need to do to manage a ref today:

<Menu
  button={({ref, ...props}) => (
    <Button {...props} ref={mergeRefs([ref, buttonRef])}>Click me</Button>
  )}
>
  ...
</Menu>

I see the con here is worth considering, where a consumer must define a custom button any time they interact with the ref.

While a fix for handling modal focus with menus is under way, this change is meant to simplify the API a bit in general.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ⚫ renders as expected in dark mode
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@geotrev geotrev requested a review from a team as a code owner September 10, 2024 21:06
onKeyDown,
disabled
}),
ref: mergeRefs([triggerRef, ref]) as unknown as RefObject<HTMLButtonElement>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ref is from Menu, which we don't want.

Copy link
Contributor

@ze-flo ze-flo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relying on consumers to use mergeRef is a win to me. 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants