Hide jump-to behind a feature flag #20069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
remove/jump-to
branch
5 times, most recently
from
32b0890
to
6a0fa40
Compare
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
remove/jump-to
branch
4 times, most recently
from
a976823
to
2c1d220
Compare
Parveshdhull
commented
May 22, 2024
| @@ -148,12 +148,11 @@ | |||
| (when-let [chat-id (:current-chat-id db)] | |||
| (chat.state/reset-visible-item) | |||
| (rf/merge cofx | |||
| (merge | |||
| @@ -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.
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.
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.
or @screen-loaded? i like more , at - atom
There was a problem hiding this comment.
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.
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.
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.
moved to constant. this will help if we want to set default stack as wallet (read some discussion in discord)
Parveshdhull
force-pushed
the
remove/jump-to
branch
from
2c1d220
to
728f326
Compare
Parveshdhull
changed the title
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 passedNot 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 passedFailed 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! |
Parveshdhull
force-pushed
the
remove/jump-to
branch
from
9bbfd58
to
fa9da76
Compare
|
@Parveshdhull, thanks for your great work! All issues are fixed, PR is ready for merge. |
Parveshdhull
force-pushed
the
remove/jump-to
branch
from
fa9da76
to
89c7c62
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #20071
Testing Note:
Known Issue:
status: ready