Wallet: Emoji picker performance #17891
Conversation
Jenkins BuildsClick to see older builds (28)
|
OmarBasem
force-pushed
the
wallet/emoji-picker-perf
branch
from
085c70e to
fe9fea5
Compare
| [{:keys [render-emojis? search-text on-change-text clear-states active-category scroll-ref theme] | ||
| :as sheet-opts}] | ||
| (let [search-active? (pos? (count @search-text))] | ||
| (rn/use-effect #(js/setTimeout (fn [] (reset! render-emojis? true)) 250)) |
There was a problem hiding this comment.
This line 158 is the only change. The rest is refactoring.
There was a problem hiding this comment.
how does it work? what was causing the issue?
There was a problem hiding this comment.
@J-Son89 rendering the emojis data is heavy on the UI thread, we need to delay rendering until the navigation animation completes
There was a problem hiding this comment.
Thanks Omar, maybe we should comment in the file about this?
Should the timeout time be a named def to help give context?
There was a problem hiding this comment.
Thanks Omar, maybe we should comment in the file about this?
@J-Son89 added comment.
Should the timeout time be a named def to help give context?
No need I think, it is used only once, and the ratom is named render-emojis? inside a setTimeout so I think it is clear what is happening
73% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
There was a problem hiding this comment.
@OmarBasem - It's not good to hardcode the sheet's animation duration in the children’s view. We should have a ratom in the bottom sheet screen (status-im2.common.bottom-sheet-screen.view ns) and update it on the actual sheet animation completion.
Also, this lag of scroll would still be present in the list if the user moves from the first tab/category to the last tab/category and tries to close the bottom sheet immediately with the pull-down gesture.
The ideal solution is to use the window-size prop in the FlatList to limit the number of items rendered off the screen and free up the UI thread if there is any sheet animation is happening.
https://reactnative.dev/docs/virtualizedlist#windowsize
We should be using the drag-gesture method to determine if there is any animation happening (probably ratom named sheet-animating?) and updating the ratom. It’s similar to disabling scroll when there is an active animation.
status-mobile/src/status_im2/common/bottom_sheet_screen/view.cljs
Lines 17 to 40 in 73e9f67
then we can pass it to the children just like other props we pass from the parent.
status-mobile/src/status_im2/common/bottom_sheet_screen/view.cljs
Lines 77 to 82 in 73e9f67
And use it in the emoji-picker to limit window-size where there is an active animation.
|
Hi @smohamedjavid thanks for your review! This PR is fixing lagging navigation animation when navigating to the emoji picker. The issue you are describing about scrolling is another problem that I think we need to open an issue for.
I am not hardcoding or changing any animation duration.
As I described above this issue is not about scrolling or gestures, but the navigation animation. Btw, in case you did not notice the only chagne in this PR is the following line 158 |
Oh, I'm sorry. It's not the scroll. It's the same lag that the user experiences when he/she switches to the last tab/category and tries to close the bottom sheet. When the user moves to the last tab/category immediately, the list renders items off the screen and it blocks the UI thread for closing animation of the sheet if the user tries to close it. RPReplay_Final1700642657.mov
I believe the
Yes, this solution is fine for now to delay the rendering of the emoji until the opening animation of the sheet is completed. But, we would experience the problems I have described above. The better solution is to update the bottom sheet screen to let the content/children know there is an active animation. |
|
@smohamedjavid the bottom sheet screen we have is a custom one. The navigation animation is not related to the gesture animation. From inside the bottom sheet screen we can know when a gesture animation is happening, but not for navigation animation (precisely, -- also the problem is local to the emojis screen). The gesture animation performance needs to be improved, but we still would need to delay rendering emojis for the navigation animation to execute smoothly. I have opened an issue #17964 As for the timeout, the |
|
Thanks for creating a separate issue @OmarBasem.
Yes, I agree it's the default for navigation transition. We have a status-mobile/src/status_im2/common/bottom_sheet_screen/view.cljs Lines 62 to 65 in 73e9f67 Apologies, if I'm not clear earlier 🙏 |
churik
force-pushed
the
wallet/emoji-picker-perf
branch
from
d249673 to
05a310f
Compare
|
@OmarBasem can you please fix lint? |
|
@OmarBasem performance is great in your PR, it is safe to merge as it touches only particular component. Ready to merge after fixing Lint. |
OmarBasem
force-pushed
the
wallet/emoji-picker-perf
branch
from
05a310f to
3d6f7a3
Compare
Thank you! |
Wallet: Emoji picker navigation perf
fixes: #17889
This PR fixes performance issues with opening emoji selector.
Before:
RPReplay_Final1699873654.MP4
After:
RPReplay_Final1699879713.MP4