-
-
Notifications
You must be signed in to change notification settings - Fork 744
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: prevent start animation when sheet is closing #205
Conversation
@heejongahn could you test this pr 👍 |
@gorhom Thanks for the quick response! I really appreciate your time. I've tested your branch with both example app and my app. I'm afraid the problem still persists. I'm not really familiar with the internals so maybe this might be totally irrelevant but: by |
@heejongahn thanks for testing it , this is weird , it seems that your anyway will also add the if-condition to the effect to prevent that too |
@heejongahn I have updated the pr, please test it again and let me know :) |
Thanks for the update!
I think this happens when
Anyway, the update seems to work fine with BottomSheetModal, but now BottomSheet shows some weird behavior :( bottomsheet.movCode sample based on example app "ADVANCED > Dynamic Snap Point" screen:
|
Also, there seems to be some kind of a timing issue with this version it there's a modal stack (i.e. when another modal should be opened again after a modal gets dismissed). This is "MODAL > Stack Modals" screen from example app. You can see that sometimes both C and B gets dismissed by clicking C, and sometimes the entire stack just goes away. stacks.mov |
@heejongahn thanks for debugging these issues 👍 for your first issue i notice that you provide const animationEasing = useMemo(() => Easing.out(Easing.quad), []); |
for the stack modals, i think we will need to create another issue for it and debug it from there |
Okay, I'll also update a example screen and send a PR.
When I checked, it seemed that the issue only happens with this branch, but not with master branch. So I thought we wouldn't want to merge this before also resolving the stack modal issue (as this might break currently well-functioning codes), and wasn't sure if filing another issue for this unmerged PR would be appropriate. If you'd like to merge this first, then I'm happy to create another issue! Either way, please let me know. |
@heejongahn good finding ! okay i'll investigate it more |
@heejongahn could you try the last commit :) |
Unfortunately, seems like same thing happens with both the example app and my app 😿 |
@heejongahn thanks alot for debugging this issue, please try the last commit , it should resolve the issue which was introduce by the first commit of this pr 😅 |
I'm not the one to be thanked... I really appreciate your time on this issue! 👍 Now the example app works fine, but my app's still suffering from the same issue. I've added some log statements for BottomSheet / BottomSheetModal methods... not-ok.movFew things I noticed:
Not sure if this would help, but this is a log when you click backdrop to dismiss the second modal: ok.movI'd really like to help yet I'm afraid I cannot pinpoint what the exact reproducing condition is, I can only suspect that it has something to do with timing issue... Please let me know if there's any way I could help. Thanks again for your effort. |
your use-case seems odd, could you share a reproachable sample code from your app or the exact same implementation on |
I'm trying to come up with the reproduction. Not there yet, but getting closer. So for my example: Second modal is select menu. Pressing one of them changes the value that first modal depends on, so eventually it makes the first modal re-render. e.g. pseudo-code: function FirstModal() {
const [firstModalInnerState, setFirstModalInnerState] = useState(defaultValue);
const onPressSecondModalItem = useCallback((newValue: numbe) => {
setFirstModalInnerState(newValue);
}, [])
// ...
return <>
// ...
<MySelect
currentValue={firstModalInnerState}
onPress={presentSecondModal}
/>
<SecondModal
onPressItem={onPressSecondModalItem}
{ /* other props */}
/>
</>;
} I tried commenting out the So I think for the stack case, it also has something to do with the not only closing but also restoring modal getting rendered. However I wasn't able to repro it within the example app (for now). I'll post here when I could come up with the minial reproducible repo. |
hi @heejongahn , could you have a look at this snack |
@gorhom just installed the fix and tested. So far no crashes 👍 Will update it something goes wrong. |
@gorhom I tried to, but the snack crashes with this error (which is odd, App.js seems fine):
I apologize that I've been bit busy at the work so didn't really have time to find exact reproduction. If there are folks waiting for this PR, and if my usecase is odd enough that merging this wouldn't cause similar issues for most users, I think merging & releasing this PR first might be a good call. I'll try to follow-up with another issue with more information on my app's issue when I found the reproduction condition. Again, thanks for your effort and time! |
Same for me, i've tried the fix and tested it. Everything OK ! |
@heejongahn the snack does crash, but for an unrelated reason. Try testing locally by updating your node modules with the fix branch |
@theohdv , @kickbk thanks for testing this pr <3 @heejongahn you are right, the snack was using the publish package not this branch ,,, my bad 😅 you could copy the code and run it on using this branch and it should work fine |
i will try to investigate #240 if it could be fix with this pr - since they are related somehow - but it is not , i will merge this one first 👍 |
found the issue of #240, it is not related to this one , however i already work on the solution and it will be pushed on another pr |
this should be fixed with |
closes #204
Motivation
this should solve the issue caused when sheet is closing and something else trigger snapping.
Installation