Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Improve Reanimated Swipeable documentation. #3121

Merged
merged 16 commits into from
Oct 2, 2024

Conversation

latekvo
Copy link
Contributor

@latekvo latekvo commented Sep 23, 2024

Description

Fixed the example provided with the Reanimated Swipeable docs, it looks like a fragment of it was missing.

Also, what's the verdict on this issue?
Today I had to take a look at these docs again and it really occurred to me the way it's currently written can be really confusing.

Here's my proposition, it removes the [0, +] mapping in favour of a verbose explanation:

renderRightActions

accepts a function.
The function receives the following arguments:

  • progress value representing swiping progress relative to the width of the returned element.
    progress is 0 when swipeable is closed, 1 when right action is fully visible.

  • translation horizontal translation of swipeable in pixels.

  • swipeable provides an object exposing the methods listed here

This function must return a ReactNode.

side-by-side comparison:

old new
image image

Of course this change applies to both renderLeftActions and renderRightActions

@latekvo latekvo marked this pull request as ready for review September 23, 2024 14:08
@m-bert
Copy link
Contributor

m-bert commented Sep 24, 2024

Also, what's the verdict on #2962 (comment)?

We could use Infinity to make it cleaner. Your suggested version does not say anything about acceptable values and I think it would be better to keep them. However, I'm still not fond of using [0, -], even with Infinity.

@latekvo
Copy link
Contributor Author

latekvo commented Sep 24, 2024

I'm still not fond of using [0, -], even with Infinity.

Please let me know if the proposal written in description is ok then, it replaces [0, -] with a regular description of the input-output ranges.

@m-bert
Copy link
Contributor

m-bert commented Sep 24, 2024

Your suggested version does not say anything about acceptable values and I think it would be better to keep them.

@latekvo
Copy link
Contributor Author

latekvo commented Sep 24, 2024

Your suggested version does not say anything about acceptable values and I think it would be better to keep them.

Do either of these work? I think the input-output range is implicitly stated in both of them.

  • progress value representing swiping progress relative to the width of the returned element.
    Equals 0 when swipeable is closed, 1 when right action is open,
    and linearly increases to Infinity as right action overshoots it's open position.

  • progress value representing swiping progress relative to the width of the returned element.
    Starts at 0, increases to 1 as right action reaches it's open position,
    keeps increasing as right action overshoots it's open position.

@m-bert
Copy link
Contributor

m-bert commented Sep 24, 2024

What about:

  • progress value representing swiping progress relative to the width of the returned element.
    Equals 0 when swipeable is closed, 1 when right action is open.
    Linearly increases to Infinity as right action overshoots it's open position.

?

@latekvo
Copy link
Contributor Author

latekvo commented Sep 25, 2024

progress value representing swiping progress relative to the width of the returned element.
Equals 0 when swipeable is closed, 1 when right action is open.
Linearly increases to Infinity as right action overshoots it's open position.

Sounds good, added it in 2967cea

docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved
docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved
src/components/ReanimatedSwipeable.tsx Outdated Show resolved Hide resolved
src/components/ReanimatedSwipeable.tsx Outdated Show resolved Hide resolved
@latekvo
Copy link
Contributor Author

latekvo commented Sep 26, 2024

As of 8c3fd7b added a missing argument description to the docstrings.
Let me know if it's ok

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to renderLeftActions also apply to renderRightActions 😅 Thanks @kacperkapusciak for help ❤️

docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved
docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved

SharedValue: [startValue, endValue]
- `progress` value representing swiping progress relative to the width of the returned element.
Equals `0` when `swipeable` is closed, `1` when left action is open.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Equals `0` when `swipeable` is closed, `1` when left action is open.
Equals `0` when `swipeable` is closed, `1` when `swipeable` is open.

Copy link
Contributor Author

@latekvo latekvo Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swipeable cannot be open, either the left action or the right action can be open.

Maybe it could be "open to the left" or "open to the right", but i think it's unnecessarily adding additional words, and i'm not sure if it resolves the issue you were referring to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

swipeable cannot be open

Why is that? For example, it has onSwipeableWillOpen and onSwipeableOpen props. What I would change here is I'd use passive voice, i.e.:

Suggested change
Equals `0` when `swipeable` is closed, `1` when left action is open.
Equals `0` when `swipeable` is closed, `1` when `swipeable` is opened.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSwipeableWillOpen distinguishes between swipeable opening it's left and right element.
Here, the distinction of which side is open is equally necessary.

What about something like this?

Equals 0 when swipeable is closed, 1 when swipeable has its left element open.

or

Equals 0 when swipeable is closed, 1 when the left element is open.


element could be replaced with action, but I think you mentioned you'd prefer to avoid action here, was that a context-specific preference or a general rule?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the distinction of which side is open is equally necessary.

Isn't it implied by the fact that we're in renderLeftActions section?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSwipeableWillOpen distinguishes between swipeable opening it's left and right element.

Yes it does, but the name indicates that Swipeable itself will be opened. Why can't you say that it is opened if you can say that it is closed?

element could be replaced with action

I think the best we could replace it with would be action panel, as in tsdocs. But still, I believe that Swipeable fits better here. (cc @j-piasecki, @kacperkapusciak)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, if we want to avoid using terms like left/right action, since these are already implied by the function name, then standalone swipeable definitely fits best.

Fixed in df0ff8c throughout 9845471

SharedValue: [startValue, endValue]
- `progress` value representing swiping progress relative to the width of the returned element.
Equals `0` when `swipeable` is closed, `1` when left action is open.
Linearly increases to `Infinity` as left action overshoots it's open position.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Linearly increases to `Infinity` as left action overshoots it's open position.
When an element overshoots the opened position the value tends towards `Infinity`. Goes back to `1` when `swipeable` is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the second sentence is a good addition: Goes back to `1` when `swipeable` is released..
But for the first one, i think the original works a bit better, it's shorter and i think the wording it uses is a bit clearer

Perhaps the following would work better?

As left action progress overshoots it's open position, the value linearly increases to Infinity.
Reverts to 1 when swipeable is released.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wanted to make it as close to natural sentence as possible. Also we'd like to avoid left action, that's why we used element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wanted to make it as close to natural sentence as possible. Also we'd like to avoid left action, that's why we used element

I believe this is resolved here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something, or the only thing that has changed is left action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something, or the only thing that has changed is left action?

I'm not sure which PR you're referencing, in df0ff8c both left and right got replaced.

Are you talking about some PR between df0ff8c and 9845471?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 620e247

docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved
docs/docs/components/reanimated_swipeable.md Outdated Show resolved Hide resolved

SharedValue: [startValue, endValue]
- `progress` - a `SharedValue` representing swiping progress relative to the width of the returned element.
- Equals `0` when `swipeable` is closed, `1` when `swipeable` is open.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Equals `0` when `swipeable` is closed, `1` when `swipeable` is open.
- Equals `0` when `swipeable` is closed, `1` when `swipeable` is opened.

WDYT about passive voice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will apply it shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in b93ec82

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time has come ⌛

@@ -29,7 +29,8 @@ import Swipeable from 'react-native-gesture-handler/ReanimatedSwipeable';

### `friction`

a number that specifies how much the visual interaction will be delayed compared to the gesture distance. e.g. value of 1 will indicate that the swipeable panel should exactly follow the gesture, 2 means it is going to be two times "slower".
a number that specifies how much the visual interaction will be delayed compared to the gesture distance.
e.g. value of 1 will indicate that the swipeable panel should exactly follow the gesture, 2 means it is going to be two times "slower".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
e.g. value of 1 will indicate that the swipeable panel should exactly follow the gesture, 2 means it is going to be two times "slower".
e.g. value of `1` will indicate that the swipeable panel should exactly follow the gesture, `2` means it is going to be two times "slower".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 65d1a2b

@@ -115,7 +120,8 @@ style object for the children container (Animated.View), for example to apply `f

### `enableTrackpadTwoFingerGesture` (iOS only)

Enables two-finger gestures on supported devices, for example iPads with trackpads. If not enabled the gesture will require click + drag, with enableTrackpadTwoFingerGesture swiping with two fingers will also trigger the gesture.
Enables two-finger gestures on supported devices, for example iPads with trackpads.
If not enabled the gesture will require click + drag, with enableTrackpadTwoFingerGesture swiping with two fingers will also trigger the gesture.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If not enabled the gesture will require click + drag, with enableTrackpadTwoFingerGesture swiping with two fingers will also trigger the gesture.
If not enabled the gesture will require click + drag, with `enableTrackpadTwoFingerGesture` swiping with two fingers will also trigger the gesture.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@latekvo latekvo merged commit f52f6f5 into main Oct 2, 2024
2 checks passed
@latekvo latekvo deleted the @latekvo/fix-reanimated-swipeable-docs branch October 2, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants