-
Notifications
You must be signed in to change notification settings - Fork 979
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
Hide jump-to behind a feature flag #20069
Conversation
Jenkins BuildsClick to see older builds (69)
|
output-2024-05-16_20.16.59.mp4 |
@Parveshdhull @pavloburykh performance are much better on my device with this build, I think we want to hide it/remove it given the perf boost until we get on top of performance |
@cammellos @Parveshdhull thank you. I will check on my device as well tomorrow. |
Hey @Parveshdhull! Performance of composer, bottom sheets is much better! Thank you! One question: I see that jump to is still displayed in chats/channels, community screen, wallet screen. I believe we need to remove it from all places. |
Also, we will need to run e2e. I suspect removing jump to button will affect our tests, but let's wait the results. cc @yevh-berdnyk |
hi @pavloburykh thank you very much for checking the PR performance.
This was just a small experiment to make sure if approach of removal of jump-to is worth it for performance. Now as we know, I will complete the PR and ping you for testing and e2e. |
32b0890
to
6a0fa40
Compare
a976823
to
2c1d220
Compare
@@ -148,12 +148,11 @@ | |||
(when-let [chat-id (:current-chat-id db)] | |||
(chat.state/reset-visible-item) | |||
(rf/merge cofx | |||
(merge |
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.
leftover?
@@ -36,16 +38,21 @@ | |||
:chat-list-scroll-y chat-list-scroll-y | |||
:chat-screen-layout-calculations-complete? | |||
chat-screen-layout-calculations-complete?}] | |||
(when-not screen-loaded? | |||
(when screen-loaded-atom? | |||
(rn/use-mount #(reset! screen-loaded-atom? true))) |
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.
we have to delay loading of chat screen otherwise blank chat is shown instead of loading skeleton as shown in video
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.
screen-loaded?-atom
i believe there should be some guidelines for atoms like https://github.com/bbatsov/clojure-style-guide?tab=readme-ov-file#dynamic-vars ,we've never used this but it's better than -atom. *screen-loaded?*
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 @screen-loaded?
i like more , at
- atom
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.
screen-loaded?
updated, thank you
or
@screen-loaded?
i like more ,at
- atom
We already have screen-loaded?
for screen-loaded? (rf/sub [:shell/chat-screen-loaded?])
in same view
(used when jump-to enabled)
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 probably I can rename this one to screen-loaded-sub? and use screen-loaded? for atom. wdyt?
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.
will leave screen-loaded?
for sub *screen-loaded?*
for r-atom. screen-loaded-sub?
seems wrong, it looks like check of sub
@@ -26,7 +27,10 @@ | |||
[rn/view {:style (style/buttons insets)} | |||
[quo/button | |||
{:on-press (fn [] | |||
(shell.utils/change-selected-stack-id :communities-stack true nil) | |||
(shell.utils/change-selected-stack-id | |||
shell.constants/default-selected-stack |
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.
moved to constant. this will help if we want to set default stack as wallet (read some discussion in discord)
2c1d220
to
728f326
Compare
ISSUE 7: Crash on iOS on login after joining a communitySteps:
video_2024-05-24_17-42-35.mp4 |
weird, I am not able to reproduce issue on simulator. Issue only happening in physical device |
22% of end-end tests have passed
Not executed tests (2)Failed tests (36)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (11)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
oh found the issue 😅. For fixing issues related to ios, I was using mac. |
@Parveshdhull may I ask you to disable it back for e2e too? Sorry for this mess, but the last test run shows that the major number of e2e tests should be aligned to the new navigation. For example when pressing back button (key code 4) the whole app is closed. So it'd be more efficient to align the tests to the final version, without jump-to. |
Yes, that was a bug. I think I fixed two commits ago. I disabled jump-to, please feel free to ping me it needs to be enabled. |
Should we give it another try, with jump-to enabled? if that will save some time? |
I run the tests for this build which is marked as number 19 here. And there was a problem with key code 4. Was is fixed later? |
yes, also I think we need to support both navigation's. If there is error after enabling feature flag, it will not look nice. Especially if it will block user |
Ok, cool. Let me run the tests agains the number 21 which is last one containing jump-to and we will see |
Good point. But it's better to have e2e builds similar to others, otherwise we can potentially miss some bugs when running the tests |
87% of end-end tests have passed
Failed tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
|
Cool! These failures are not PR related. So let's leave jump-to in e2e builds as it is for now. I'll turn it off and fix the tests accordingly in #20153 when this PR is merged. Thanks @Parveshdhull! |
9bbfd58
to
fa9da76
Compare
@Parveshdhull, thanks for your great work! All issues are fixed, PR is ready for merge. |
fa9da76
to
89c7c62
Compare
fixes #20071
Testing Note:
Known Issue:
status: ready