software-mansion / react-native-gesture-handler

Declarative API exposing platform native touch and gesture system to React Native.
https://docs.swmansion.com/react-native-gesture-handler/
MIT License
6.13k stars 982 forks source link

Forward unused props in `ReanimatedSwipeable` and `Pressable` to their inner components #3039

Closed latekvo closed 3 months ago

latekvo commented 3 months ago

Description

Currently large portion of the props accepted by ReanimatedSwipeable and Pressable components remain unused. This issue mostly refers to all the accessibility props.

This PR adds the use of spread operator to ReanimatedSwipeable and Pressable to forward all the remaining props to inner components, which are not explicitly used otherwise.

Fixes: #3032

Test plan

latekvo commented 3 months ago

I just found that using the following code works just as well as the ManagedProps method:

const { children, style, ...rest } = props;

The only downside being that it requires to pull out props before being used, as opposed to accessing them directly from the props object.

And so if we've got 20 unique props in one of our components, that'll require having a 20-line-long header of extracted props as opposed to pulling them on-the-go.

@j-piasecki @m-bert Would we prefer to stick with the ManagedProps method, or remove it in favour of this new one?

m-bert commented 3 months ago

ManagedProps looks like using a sledgehammer to crack a nut. In my opinion using spread syntax is enough.

And so if we've got 20 unique props in one of our components, that'll require having a 20-line-long header of extracted props as opposed to pulling them on-the-go.

I think that it is unrealistic edge-case that we shouldn't be concerned about.

Also, I have a couple of comments on the code, but first let's see if @j-piasecki agrees with me 😅

latekvo commented 3 months ago

I think that it is unrealistic edge-case that we shouldn't be concerned about.

i believe it's the exact case this PR is dealing with. The 2 components utilising ManagedProps are ReanimatedSwipeable and Pressable, both using 16 and 17 unique props respectively.

Although with all that said, I think spread operator approach is indeed simpler and more readable than ManagedProps.


Converted to spread operator approach in 0ee242f.

m-bert commented 3 months ago

i believe it's the exact case this PR is dealing with.

Oh okay, what I had in mind was only passing props to components, I forgot about the whole implementation dealing with different props. Nevertheless I believe that spread syntax is better 😅