-
Notifications
You must be signed in to change notification settings - Fork 286
fix: Check Panning for AutoHide state. #574
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
Conversation
DetailsThis package uses "Pan Responder" to handle "Pan Gestures". In this case, there is a limitation. Toast component accepts "Customizable Components" from the user. If "Customizable Component" contains a "Touch or Click" and the user only interacts with this "Touch or Click" and only holds it, "Pan Responder" will not capture any touch action. In this case, the "panning" value in this pull request cannot be changed. Therefore, the old problems continue. There are 2 different things that can be done to prevent this situation;
I implemented a temporary method to get rid of the side effects of touch actions in these child components and to prevent state incompatibility. And here is what the difference between the old version and the new version looks like. Samples
ScenariosScenario 1: User shows Toast. Toast is hidden after a certain time. Scenario 2: User shows Toast and taps and holds on Toast area. Scenario 3: User shows Toast and taps and holds on touchable child component inside Toast. |
|
#488 This pull request is a bit dangerous. If there is any side effect, it will cause a "Non-Interactive Toast". Therefore, this code should be reverted. "Toast is Visible on Screen" but "isVisible=false" -> "Non-Interactive Toast" The pull request made here will already eliminate most of the "Side Effects". However, it should still be reverted as a precaution. |
|
@HyopeR Thanks for taking the time to do this! Please check the test suite, some test cases are failing. |
|
Some of the checks that did not pass the tests seem to be buggy outside of this pull request. I don't know much about tests. But I can't run tests locally. |
|
To be able to run them locally, you need a setup similar to what's on CI (a setup that is long overdue for an update, I must add). You can see the details here (node, react, react-native versions, etc). Note: after you yarn add react and react-native locally, make sure to discard the changes to package.json / yarn.lock. -- One more thing, the coverage went down on this branch, would you be able to add tests to cover the new code paths? Or do you need my help? Thanks again! |
|
I was able to run the tests by installing the react and react-native versions mentioned here and downgrading the node.js version to 16. The coverage appeared to be 100%. More guidance on ".lock" problems and "testing" processes would be really nice to make contributing to the package a bit easier. |
|
If there is anything that needs to be changed, let me know. I would like to continue with the version instead of the fork. |
|
All good, I'll merge and publish it in a new version. Thank you for doing this! |
Detail
Check the "panning" status of the user in "Auto Hide" state. If the user is in "interacting" state, skip the auto-closing process.
Incompatibility between the state and the interface can lead to many side effects, such as corrupted touch events, empty space coverage, or toast that cannot be removed.
Related Issues
#531, #562
What's happening?
What has changed?