Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[docs] Improve Reanimated Swipeable documentation. #3121
Changes from 5 commits
8f6c426
2967cea
c5ed032
6f68b59
8c3fd7b
e37a656
6cdf23b
abf3849
b840aa3
df0ff8c
3eb201e
9845471
b93ec82
449fa48
620e247
65d1a2b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 theleft action
or theright 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that? For example, it has
onSwipeableWillOpen
andonSwipeableOpen
props. What I would change here is I'd use passive voice, i.e.:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onSwipeableWillOpen
distinguishes betweenswipeable
opening it's left and rightelement
.Here, the distinction of which side is open is equally necessary.
What about something like this?
or
element
could be replaced withaction
, but I think you mentioned you'd prefer to avoidaction
here, was that a context-specific preference or a general rule?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it implied by the fact that we're in
renderLeftActions
section?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?I think the best we could replace it with would be
action panel
, as intsdocs
. But still, I believe thatSwipeable
fits better here. (cc @j-piasecki, @kacperkapusciak)There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 usedelement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is resolved here
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 620e247