react-spectrum icon indicating copy to clipboard operation
react-spectrum copied to clipboard

useButton should accept and pass `continuePropagation` to the `usePress` hook.

Open pedroapfilho opened this issue 1 year ago โ€ข 6 comments

Provide a general summary of the feature here

Pass continuePropagation to useButton, and pass it down to usePress, so the Buttons created with useButton can have events propagated to parents with the onPress.

๐Ÿค” Expected Behavior?

continuePropagation should be a prop for useButton, that should be passed to usePress

๐Ÿ˜ฏ Current Behavior

Currently there's no way to pass continuePropagation to the useButton, so it can be used by usePress.

๐Ÿ’ Possible Solution

I saw the implementation of useButton, and saw that it uses usePress, and that usePress has continuePropagation, but it's not used by useButton. so my proposal is to pass the continuePropagation to useButton and pass it down to usePress, so everything works as expected on these edge cases.

๐Ÿ”ฆ Context

We currently use the useButton to create part of our design system, and there are some cases when we use our button with other libraries eg: flexlayout-react, and the library expects the event to bubble to their own button, so the event can be triggered.

๐Ÿ’ป Examples

No response

๐Ÿงข Your Company/Team

No response

๐Ÿ•ท Tracking Issue

No response

pedroapfilho avatar Apr 05 '24 13:04 pedroapfilho

You can actually call e.continuePropagation in the press event handlers you pass to useButton to accomplish this: https://codesandbox.io/s/autumn-glade-mlkdxp?file=/src/App.js. However I'd be careful doing this since relying on the event bubbling all the way though can be brittle, see https://github.com/adobe/react-spectrum/issues/2100 for some discussion around this

LFDanLu avatar Apr 05 '24 23:04 LFDanLu

It seems like continuePropagation is not part of the PressEvent type, is it intended?

pedroapfilho avatar Apr 08 '24 12:04 pedroapfilho

Thats odd, ButtonProps extends from https://github.com/adobe/react-spectrum/blob/68af9238609422cbeecd5e18cb74c9a9d3ef1052/packages/%40react-types/shared/src/events.d.ts#L33-L54 which has it. Shows up locally for me as well as in the docs which get their props from the types: https://react-spectrum.adobe.com/react-aria/usePress.html#usage. Do you have a sample repro?

LFDanLu avatar Apr 08 '24 17:04 LFDanLu

You're right, for some reason my editor wasn't auto-completing it, and I was blind to the property on the bottom. I'm sorry.

Just out of curiosity, why did you implement the e.continuePropagation on the onPressStart, and not on the onPress?

pedroapfilho avatar Apr 09 '24 13:04 pedroapfilho

No worries! The reason the e.continuePropagation is only on onPressStart is because if the user doesn't continue propagation onPressStart then the events gets stopPropagated at that point and won't propagate to the parent, meaning its too late to try to continuePropagation in onPress.

LFDanLu avatar Apr 09 '24 16:04 LFDanLu

One fun fact is that integrating it with react-router-dom NavLink has some gotchas. It would be good to have some examples of both working together.

pedroapfilho avatar Apr 17 '24 11:04 pedroapfilho