fix: use a new hook to combine the refs in Popup rather than overriding the passed ref

[trevinhofmann]

This was a fun one to debug 😄

I eventually noticed that the passed popupRef always had a current value of null, so it wasn't being set even though the Popup component seemed to correctly use React.forwardRef. Later, I noticed that the useSwipeToDismissProps also have a ref prop. That was overriding the initial ref={ref} because we were spreading all of the useSwipeToDismissProps including the new ref.

I could have simply moved the ref={ref} line below {...(isTouch ? useSwipeToDismissProps : null)}, but then we would be overriding/losing the ref from useSwipeToDismissProps which is almost certainly not desirable either.

To solve this, I created a new hook called useCombinedRefs which lets us create a new ref combining multiple other refs. In Popup, we now use that hook to combine the passed ref with useSwipeToDismissProps.ref.

Now, with the ref being set, the CSS animation styles are being correctly applied to the popup in the gesture menu.

This fixes #2059.

Demo:

Screen.Recording.2024-08-16.at.3.35.18.PM.mov

[raineorshine]

Interesting! Tricky indeed. I'll have to remember to watch out for spreading props.

[trevinhofmann]

Interesting! Tricky indeed. I'll have to remember to watch out for spreading props.

FWIW, I don't even allow spreading JSX props in my own ESLint config: https://github.com/trevinhofmann/eslint-config-principled/blob/08b1601df2a7f7d4db734aec708d410685389602/src/components/react/best-practices.ts#L71-L74