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

Wrong fire onSwipeableWillOpen #935

Closed
dmytro-kupriianov opened this issue Jan 28, 2020 · 7 comments · Fixed by #2589
Closed

Wrong fire onSwipeableWillOpen #935

dmytro-kupriianov opened this issue Jan 28, 2020 · 7 comments · Fixed by #2589

Comments

@dmytro-kupriianov
Copy link

Method onSwipeableWillOpen fired only after open panel, but not when start movement. How fix this moment?

@MoOx
Copy link

MoOx commented Oct 28, 2020

Having the same issue. I would have expected that this event is called on start. How this differ from onSwipeableOpen then?

@MoOx
Copy link

MoOx commented Oct 28, 2020

The idea in #1101 could be a solution if changing onSwipeableWillOpen is not an option (eg: undesired breaking change).

@jakub-gonet jakub-gonet linked a pull request Nov 24, 2020 that will close this issue
@jakub-gonet
Copy link
Member

jakub-gonet commented Nov 24, 2020

#999 should make this possible now.

@MoOx
Copy link

MoOx commented Nov 24, 2020

Not really as discussed in #1102. #999 is for Drawer only as we are here discussing about Swipeable.

@jakub-gonet jakub-gonet reopened this Nov 24, 2020
@jakub-gonet jakub-gonet removed a link to a pull request Nov 24, 2020
@MoOx
Copy link

MoOx commented Nov 24, 2020

Btw, what is the difference between onSwipeableWillOpen and onSwipeableOpen. If there is a good reason, I think new lifecycle method are necessary. If there is no good reasons (= not really a bug but still is?) it should be changed, but would be a breaking change. Otherwise, we could add new lifecycle (onSwipeStart+End) as onSwipeableWillOpen might be legit for programmatic opening as you mentioned in #1102 (comment).
I am open for a documented PR if we agree on something :)

@jakub-gonet
Copy link
Member

Btw, what is the difference between onSwipeableWillOpen and onSwipeableOpen.

I'd want to know it myself. We can check it in the code but that's not the point – having will/did and plain lifecycle methods is confusing to the developers using this component.

Currently, we have the following description in docs:

onSwipeableOpen

method that is called when action panel gets open (either right or left).

onSwipeableWillOpen

method that is called when action panel starts animating on open (either right or left).

I imagine this is confusing to the new developers using RNGH. onSwipeableWillOpen gets called before opening but at the start of the animation? onSwipeableOpen gets called when opened swipeable or in the same time when will variant? What about people who use TS as their documentation?

Otherwise, we could add new lifecycle (onSwipeStart+End) as onSwipeableWillOpen might be legit for programmatic opening as you mentioned in #1102 (comment).

For me, there's little sense to create another lifecycle method; we added will/did, left/right prefix and it effectively makes the number of lifecycle methods eightfold. Adding onSwipeStart/End makes situation worse.

Ideally, it'd be only one/two callbacks:

  • onSwipeableOpen taking a function with two args: (direction, animating)
  • onSwipeableOpen similar to one above

Or:

  • onSwipeableStateChange taking function with (direction, animating, isOpen)

We could add another method similar to #999 providing slide percentage as well(have you almost finished swiping?).

But we have to work with what we have so I think adding something that was mentioned in #1101 could be beneficial (you can just make a guard for other positions: if(position !== 0) { return; } // (...). It'd be a bit less efficient than the dedicated state change method but it should be good enough.
Then we can add new methods and deprecate older so functionality stays the same and people will have time to transition between them.

@MoOx
Copy link

MoOx commented Dec 29, 2020

onSwipeableWillOpen is incorrectly documented.
Currently (I just tried to redo the same thing I did on another project without my patch/hack from another PR) and onSwipeableWillOpen fire when:

  • you have the thing opened
  • AND when you release your finger
    Then, a few MS later, when animation is totally ended, onSwipeableOpen fires.

Currently I am trying to implement the dumbest thing possible for swipeable: being able to have a row with open options.
Just a single row. This means, as soon as you start swiping another row, the opened row closes. It's a behaviour you can find on all iOS swipeable list in mail, messages etc. Super classic thing.
Without a patch with a new lifecycle method in Swipeable, this is just impossible to call some code when a swipe is started by the user, even if that's just a few pixels.

I am open to any clean solution that allow this trivial behaviour to be implemented.

m-bert added a commit that referenced this issue Sep 20, 2023
)

## Description

Fixes #935 
The event onSwipeableWillOpen only starts right before the
onSwipeableOpen event, not when the drag to open / close starts. This PR
includes onSwipeableOpenStartDrag (direction: 'left' | 'right') and
onSwipeableCloseStartDrag (direction: 'left' | 'right'), allowing
execute actions when the animation to open/close starts (when drag
starts).

## Test plan

Tested in the example app.

---------

Co-authored-by: Felipe <felipe.farias@gazin.com.br>
Co-authored-by: Michał Bert <63123542+m-bert@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants