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

fix: get properties from top-most screen on swipe #1509

Merged
merged 8 commits into from
Jul 8, 2022

Conversation

kacperkapusciak
Copy link
Member

@kacperkapusciak kacperkapusciak commented Jul 4, 2022

Description

This PR fixes an issue on which swipeDirection was required to be set on both on current and previous screen because the value after one tick was incorrectly taken from the previous screen.

Thanks @WoLewicki for the solution 👏

Screenshots / GIFs

Before

Screen.Recording.2022-07-04.at.11.41.47.mov

After

Screen.Recording.2022-07-04.at.11.40.11.mov

Test code and steps to reproduce

TestsExample/Test1509.tsx

Checklist

  • Included code example that can be used to test this change

kacperkapusciak and others added 2 commits July 4, 2022 11:36
when we were swiping with the current logic in the next frame properties were taken from previous screen
Co-authored-by: Wojciech Lewicki <wojciech.lewicki@swmansion.com>
Co-authored-by: Wojciech Lewicki <wojciech.lewicki@swmansion.com>
@kacperkapusciak kacperkapusciak marked this pull request as ready for review July 4, 2022 11:03
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

Good job 🎉

@hirbod
Copy link
Contributor

hirbod commented Jul 4, 2022

@WoLewicki do you still remember the fullScreen swipe issue I've showed you at App.js conf?
When you swipe on the edge, it will trigger the default swipe gesture instead of the custom for fullScreen.

Maybe thats a good point to fix this as well :D

@WoLewicki
Copy link
Member

@hirbod good point! I'll look into it. Do you have a quick repro for this and could paste it even here?

@hirbod
Copy link
Contributor

hirbod commented Jul 5, 2022

@WoLewicki this should work
https://snack.expo.dev/@hirbod/swipe-pop-issue

I could not reproduce this in Expo Go though.

Here is a video from my app. Depending on where I swipe (edge) or somewhere in the screen, it will have a shadow left or not. Its not a big deal though.

RPReplay_Final1657010606.mp4

@kkafar
Copy link
Member

kkafar commented Jul 6, 2022

Hi @hirbod
I'm looking at your snack right now & that's right :D Depending on where your gesture starts it is handled by different gesture recognisers. When you start the gesture near the edge native animation is being played, otherwise animation defaults to 'simple_push'. So if you want to have the same effect on whole screen you should try to set animation: "simple_push".
However I see that this is not the best behaviour, and we could maybe set the animation to 'simple_push' by default.

@hirbod
Copy link
Contributor

hirbod commented Jul 6, 2022

That might be an option. A third option would be to have 'simple_push_with_shadow' :D

@WoLewicki
Copy link
Member

But still the native header transition is only possible with the default gesture recognizer 😢

@hirbod
Copy link
Contributor

hirbod commented Jul 6, 2022

Yeah the header is not that bad though, the shadow is also not a big deal breaker - just a visual thing

kkafar and others added 2 commits July 7, 2022 16:01
Submitted in #1514 by @hurali97

Co-authored-by: Muhammad Hur Ali <hurali87@gmail.com>
Merge branch 'main' into @kacperkapusciak/get-correct-screen-on-swipe
@kkafar kkafar merged commit 42c4e48 into main Jul 8, 2022
@kkafar kkafar deleted the @kacperkapusciak/get-correct-screen-on-swipe branch July 8, 2022 07:11
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.

4 participants