-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[NoQA] fix: bottom tab transitions duration #53754
[NoQA] fix: bottom tab transitions duration #53754
Conversation
@dannymcclain @DylanDylann One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@mountiny may I ask you to trigger a build and ask design team to make tests to be sure that the duration is okay now? 😊 |
🚧 @dannymcclain has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
@DylanDylann @kirillzyusko Could you please add recordings here? I cannot run the ad-hoc build unfortunately. |
Sure, I will do it soon |
Ooops, I somehow uploaded wrong recordings 😔 I uploaded correct recordings now! |
@kirillzyusko the test failed |
9aa2a0d
to
8b29e5e
Compare
@DylanDylann tests fixed! |
@DylanDylann, what is your eta for this one? |
@dannymcclain Could you make a build again and test on that build to ensure the duration is fine now? |
From my testing, I don't see a significant difference in the behavior compared to the main branch when clicking on bottom bar tab |
@DylanDylann did you make a clean installation of |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Would love to have @dubielzyk-expensify and @shawnborton test the latest build too. Testing the iOS ad hoc build, it still feels a little slow to me? It also kinda feels like the transition to the search tab takes longer than the other tabs... but maybe that's just in my head? It doesn't feel bad to me, but it doesn't feel just right either. Generally I think it's shippable but could still use some improvement. |
This is a separate issue. We tracked it here #53818 This PR just focuses on reducing animation duration (I haven't optimized slow rendering of Search page in this PR, we decided that it would be better to solve different problems in different PRs). |
@kirillzyusko Ahh thanks, I had the same thought with @dannymcclain |
Hmm I think things are still broken? I was consistently tapping back and forth between Inbox and Settings and then the bottom tabs became totally unresponsive and now I can't change the tabs anymore. Can you reproduce on your end too? |
When it works though, I think it feels better? Honestly I wouldn't hate it a tad faster though I think. |
import type {PlatformSpecificNavigationOptions} from '@libs/Navigation/PlatformStackNavigation/types'; | ||
import Animations from '..'; | ||
|
||
const fade: PlatformSpecificNavigationOptions = {animation: Animations.FADE, animationDuration: 150} satisfies NativeStackNavigationOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be curious to try 100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather, what is the current animation/transition duration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather, what is the current animation/transition duration?
I think at the moment it's 500ms, but visual perception is about 350ms (it's a spring animation and last 150ms we may just animate opacity from 0.95 to 1, which might be not an actual duration perception).
Do you want me to change it to 100ms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shawnborton @dannymcclain @dubielzyk-expensify I will now go ahead with this change, we can play around with the delay based on how it will feel later
Tested it on both Android and iOS:
IMG_1992.MOVPart of me wonders if we should set the animation to like 50ms or lower (or even remove it), then figure out the timing of the tap -> animation start -> animation end. That way we can figure out what feels sluggish. |
Yes, because Android are typically less optimized and works slower comparing to iOS.
Yeah, for testing purposes we can set duration to 0 or completely remove the animation. But again, optimization of transitions should be made in a separate PR. The current problem is that after native-stack PR merge screens in bottom tabs are getting re-created whenever user changes them, but these screens should be created just once and then we should just change Maybe @chrispader can shed more light on why the behavior has been changed 👀 |
I think we've been discussing this at some point in the Native Stack PR too, i think this delay is mostly due to internals in the Native Stack implementation compared to stack navigator.
From what i've seen, i agree with @kirillzyusko, i think this can mostly be seen for the SearchPage which is especially slow to render. What we can try is loading the bottom tab screens statically instead of lazily ( |
Potentially problem with slow tab switching has been fixed in #54030 |
Testing it on iPhone 15 Pro and I'm still seeing a pretty big lag. I don't know the technicals deeply but what you're all saying above sounds worth looking into |
@kirillzyusko If that, we should mark this PR as NO QA and move test step to #54030 |
@kirillzyusko When clicking on the search tab and then clicking on the settings or chat tab, it is unresponsive Screen.Recording.2024-12-17.at.15.11.53.mov |
@DylanDylann the same problem exists on main as well, so I don't think it should prevent this PR from being merged: unresponsive-tabs-on-web.mp4 |
@DylanDylann can you please complete the checklist and testing? |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-18.at.12.27.45.movAndroid: mWeb ChromeScreen.Recording.2024-12-18.at.12.17.18.moviOS: NativeScreen.Recording.2024-12-18.at.12.27.30.moviOS: mWeb SafariScreen.Recording.2024-12-18.at.12.16.16.movMacOS: Chrome / SafariScreen.Recording.2024-12-18.at.12.15.17.movMacOS: DesktopScreen.Recording.2024-12-18.at.12.18.17.mov |
All good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
import type {PlatformSpecificNavigationOptions} from '@libs/Navigation/PlatformStackNavigation/types'; | ||
import Animations from '..'; | ||
|
||
const fade: PlatformSpecificNavigationOptions = {animation: Animations.FADE, animationDuration: 150} satisfies NativeStackNavigationOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shawnborton @dannymcclain @dubielzyk-expensify I will now go ahead with this change, we can play around with the delay based on how it will feel later
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
2 similar comments
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.78-0 🚀
|
Explanation of Change
Reduced animation transition to 150ms to make it faster.
Fixed Issues
$ #53347
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
telegram-cloud-document-2-5384371478454101952.mp4
Android: mWeb Chrome
Screen.Recording.2024-12-09.at.14.32.01.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-12-09.at.14.29.35.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-12-09.at.14.33.26.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-12-09.at.14.37.15.mov
MacOS: Desktop
Screen.Recording.2024-12-09.at.14.46.03.mov