feat(react): support react 17

[anastasialanz]

closes #1473

This PR does a few things:

• For the OptionsMenu and Popover component, when a user opens them, clicking on the trigger buttons will now close them instead of triggering the click outside close() method
• Converts the OptionsMenu components to function components
• Updates the TopBar docs so that the OptionsMenu now has a trigger to reference

Verification Steps

• Go to the OptionsMenu component
  • Open the OptionsMenu
  • Clicking on the trigger for the OptionsMenu will close the OptionsMenu
  • Verify that the click outside handler works
• Go to the Popover component
  • Open the Popover
  • Clicking on the trigger for the Popover will close the Popover
  • Verify that the click outside handler works
• Go to the TopBar with menu component
  • Open the OptionsMenu inside the TopBar
  • Clicking on the trigger for the OptionsMenu will close the OptionsMenu
  • Verify that the click outside handler works
• Go to the Modal component
  • Verify that the Modal component opens and closes with the trigger

[aws-amplify-us-east-1]

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1680.d15792l1n26ww3.amplifyapp.com

[anastasialanz]

From Jason:

The changes here I suspect are related to testing-library/user-event#592, but we're not using user-event but are calling fireEvent due to specific limitations with testing library.

[anastasialanz]

We don't need to add any other elements to test clicking outside. We can click on the body element instead.

[anastasialanz]

Small cleanup since these types were repeated.

[anastasialanz]

Typo!

[anastasialanz]

Same typo as above.

[anastasialanz]

These changes are recommended here https://legacy.reactjs.org/blog/2020/08/10/react-v17-rc.html#fixing-potential-issues

[scurker]

We'll want to be sure we call this out in the release, might be good to tag folks in the slack channel. I don't think anyone should be relying on the capture vs bubble phase but you never know...

[scurker]

Everything looks pretty good, but I do think we need to address the breaking change here.

[scurker]

We'll want to be sure we call this out in the release, might be good to tag folks in the slack channel. I don't think anyone should be relying on the capture vs bubble phase but you never know...

[scurker]

Unfortunately, this appears to be a breaking change. Does this change still need to be made even when we're checking the event in the capture phase?

[anastasialanz]

This change is needed because the trigger button is outside of the ClickOutsideListener component which caused the menu to not be able to close after it was opened. If the menu was open and you clicked on the trigger button to close it, it would call handleClickOutside in OptionsMenuList and then toggleMenu in the OptionsMenu would be called to keep it open again. That's why I do the check below to not call onClose within handeClickOutside if we click on the trigger element:

const handleClickOutside = (e: MouseEvent | TouchEvent) => {
    const target = e.target as Node;
    const triggerElement = triggerRef.current;
    if (target === triggerElement || triggerElement?.contains(target)) {
      return;
    }

Let me know if you think there's a better way to go about this.

[scurker]

I pulled down your PR, and I have a few thoughts about this.

One way we could handle this in a non-breaking manner, is to check for event.defaultPrevented here:

cauldron/packages/react/src/components/OptionsMenu/OptionsMenu.tsx

Lines 48 to 50 in 9df13e6

	const toggleMenu = () => {
	setShow(!show);
	};

Within the click outside callback we could then set event.preventDefault if the menu list is currently shown. This does come with the downside of it would prevent immediate clicks on elements outside of the menu while the list is open so not the best solution, but also non-breaking.

Aside from that, we need to consider these components could also be constructed piecemeal so we need to accommodate for that scenario as well. Instead of explicitly trying to set the ref on a component (and not all components correctly implement the ref property) we might want to consider capturing the element that triggered the open state:

const triggerRef = useRef<HTMLElement>()
const toggleMenu = (event) => {
  if (!show) {
    triggerRef.current = document.activeElement
  } else {
    triggerRef.current = null
  }
  setShow(!show);
};

OptionsMenuList could optionally accept the ref (to be non breaking):

export interface OptionsMenuListProps
  extends Omit<OptionsMenuProps, 'trigger'> {
  className?: string;
  triggerRef?: React.RefObject<HTMLElement>
}

...

const handleClickOutside = (e: MouseEvent | TouchEvent) => {
  if (triggerRef && triggerRef.current) {
    // use existing checks
  }
}

IMO it makes it slightly more robust without there needing to be a breaking change. Thoughts?

[scurker]

In the long run, we're on the path to create a brand new component that will replace this one and deprecate usage of this component. It has a lot odd patterns that do not support some of the mechanisms we're trying to achieve and any changes we would make would need to be breaking. A brand new component to move away from some of these patterns without breaking immediate usage is the path of least resistance.