Popper: remove findRefWrapper

[kmcfaul]

trafficstars

What: Closes #6050

Popover and Tooltip work as usual, however there is currently an issue with using the Popper directly (shown in the composable menu demos) where the state/position isn't updating as frequently as before that I am still looking into. For instance, scrolling with Popover causes continuous state updates to the position while the menu demos do not update at all so the popper menu is scrolling out of frame instead of updating to the trigger position.

@jschuler Do you have any thoughts why this may be? I had removed the ref state to just use refs directly so there are less internal state updates, or maybe one of the various memoizations needs to be updated?

[patternfly-build]

Preview: https://patternfly-react-pr-7807.surge.sh

A11y report: https://patternfly-react-pr-7807-a11y.surge.sh

[jschuler]

What if we left the old API in place, and added a new option for consumers to use who want to pass react strict mode? Example:

Popper.tsx

return (
    <>
      {!reference && trigger && typeof trigger === 'object' && (
        <FindRefWrapper onFoundRef={(foundRef: any) => setTriggerElement(foundRef)}>{trigger}</FindRefWrapper>
      )}
      {!reference && trigger && typeof trigger === 'function' && trigger(setTriggerElement)}
       ... same with the popper element

then in code:

old way

<Popover
    aria-label="Basic popover"
    headerContent={<div>Popover header</div>}
    bodyContent={<div>Popovers are triggered by click rather than hover.</div>}
    footerContent="Popover footer"
  >
    <Button>Toggle popover</Button>
  </Popover>

new way

<Popover
    aria-label="Basic popover"
    headerContent={<div>Popover header</div>}
    bodyContent={<div>Popovers are triggered by click rather than hover.</div>}
    footerContent="Popover footer"
  >
    {(setTriggerElement) => <Button ref={setTriggerElement}>Toggle popover</Button>}
  </Popover>

[kmcfaul]

PR updated -

• Changes to utilize wrapping divs now require the removeFindDomNode flag (Popper, and components that use Popper have this new prop).
• Functions may also be passed in place of direct elements for trigger and popper menu, but are not completely ironed out yet (composable menu demo keyboard behavior is breaking and position is a bit wonky). If we run out of time but want the opt-in to be merged we can stash those changes and revisit it later on as well.
• Snapshot tests and integration tests are reverted to previous state as by default nothing is changing now (I kept one integration test update in to enforce a specific viewport size so the test won't break randomly if the viewport ends up too large/small which will change the expected text).

[kmcfaul]

Opened #7872 to track a future breaking change to enable the wrapping divs by default.

[kmcfaul]

@jschuler Updated the typings and also added a strict mode example. I was able to get the Menu keyboard handling to work by calling the setPopperElement inside the passed function instead of passing the callback down into Menu. Let me know there are any issues, I think I noticed the initial state not being updated but every subsequent reopening of the menu is working.

Added the beta flag to all the removeFindDomNode props as well.

[tlabaj]

@kmcfaul looks good to me. It would e good to add a unit test to verify in the various components.

[kmcfaul]

Resolved the handful of demos that were no longer rendering due to a syntax error after the rebase. I'm not sure why, but several examples having an optional prop was causing the issue? And not all examples have this issue either, so I'm not sure if that or something else is off.

[jschuler]

Noticed that I had to click multiple times on the trigger for the menu to show in this example https://patternfly-react-pr-7807.surge.sh/demos/composable-menu/#composable-strict-mode-dropdown

[kmcfaul]

@jschuler Yeah I've been on and off trying to debug that. The example uses functions to bypass the findDOMNode call, which I've tweaked to allow keyboard navigation to continue to work. However that change seems to be causing the initial state to be off. Any thoughts? I could change it back to the previous setup but keyboard navigation wasn't working due to the ref being set to a function (which wasn't working with Menu).

[kmcfaul]

I've removed the functional version of bypassing the strict mode error for now as the initial position is not updating properly. We can revisit this method in the future if you see no issues benching it for now @jschuler

Added popperRef method. Removed the separate strict mode example. The solution may be tested on other examples wrapping the render with the React.StrictMode element and adding the appropriate props. Added various comments and descriptions to the Popper codebase.

The strict mode error currently may be resolved through the following options:

• removeFindDomNode flag is defined. This will cover both popper and trigger errors.
• popperRef (and popper) and reference properties are defined. Without the flag, the reference properties of both elements must be provided.
• A mixture of removeFindDomNode and reference properties. This is more pertaining to the trigger, as the trigger is not required if the reference is passed in. For the popper, popper is still required in addition to the flag or reference.

[patternfly-build]

Your changes have been released in:

• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• @patternfly/[email protected]
• [email protected]

Thanks for your contribution! :tada: