fix: activity center closing animation #15222
Conversation
fix: activity center closing animation lint lint
OmarBasem
force-pushed
the
fix-popover-animation
branch
from
253b00d
to
e29d5de
Compare
There was a problem hiding this comment.
You did remember to fix the springy animation! Thank you 🎆
I still find the bottom sheet opening animations too slow when using the mobile app, but it's not something we should worry in this PR. I usually grab a cup of coffee between bottom sheet opening and closing animations ☕ 😉
Gone with the spring!
@ilmotta HAHAHA! 😃
This PR improves it a bit, but still far from perfect. As we discussed before, it would have been better if we used a screen, but of course due to the design constraints a screen may not give the intended look. It looks okay on iOS, but on Android, for some reason, there is some delay between the initial screen overlay appearing and the activity center view animating. We can investigate further in another issue. |
@OmarBasem, this is a very important topic you're touching. Just today I explained to Andrey why this delay exists and how it could be eliminated. I'm gonna quote myself:
|
|
Do you know why we have a fixed delay of 250ms to render popovers? https://github.com/status-im/status-mobile/blob/develop/src/status_im/popover/core.cljs#L9 This delay is far too long as Omar and Andrey recently reported. Delays above 100ms are easily perceived as lag by humans. I'm pretty sure there's a reason for this, but if I lower it to say 30ms, opening the AC feels snappy, way better. I'm assuming the @Parveshdhull, if you think the 250ms is necessary, I'll change the popover event to accept a Edit: cc'ing @flexsurfer who might have context as well |
|
i don't remember :( probably not needed anymore, but I'm more wondering why is it used for AC |
You mean the popover? The popover is needed due to the requirement to display the blurred (live) background behind. I was even in a meeting with John discussing about this. It seems we can't use a screen, and if performance becomes an issue in the future an external RN expert will be considered to implement/optimize screens so that we can blur the screen behind without resorting to popovers (this external contributor could be the same one working on the optimized flatlist). This "blur problem" will exist for the new Settings "screens". The designs require live & blurred backgrounds, but I talked to designers and John and the requirement to keep the blur is non-negotiable 🤷♂️ |
|
Btw, why do we have both popovers and bottom-sheets? I think both can achieve the same effect. Doesn't this create a bit of confusion? |
|
Answering a little bit my own question. It seems that the popover is only properly displayed with a bit higher delays, like 10+ ms. So I guess someone decided to use 250ms to play on the safe side. |
Confusion might be very personal. But I can speak for myself, yes, I'm confused by the terminology. |
most likely it was me , popover was a really rare case, so i decided to play safe |
Did the right call ;) I'll open a PR, but I'll keep the default 250ms @flexsurfer. I'm not in the mood of introducing weird regressions in other places and in some devices if I lower it too much. |
|
i think i remember why, because popover react view has an animation itself, we use RNN popover only as a container and it doesn't have an animation, yeah right i even left a comment |
I saw the comment before, but I was still left wondering why the hardcoded high delay. Now we know, thanks. |
Make the popover delay in milliseconds configurable. To open the Activity Center, 30ms delay seems to be more than enough. It's a quick fix because the current 250ms to open the AC gives the wrong impression to users (and confuses even us developers) who think the AC has a performance issue to open. See issue #15222 for the full discussion.
fixes: #15221