Onboarding - New opt-in usage data screen #21655
Open
+181
−176
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 (28)
|
Parveshdhull
force-pushed
the
fix/onboarding-usage-data-screen
branch
6 times, most recently
from
f8ccd62
to
3c49d9a
Compare
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
fix/onboarding-usage-data-screen
branch
from
3c49d9a
to
d62975f
Compare
Parveshdhull
commented
Nov 25, 2024
Comment on lines
+81
to
+83
| parent (or (first @state/modals) @state/root-id)] | ||
| (navigation/push | ||
| (name @state/root-id) | ||
| (name parent) |
There was a problem hiding this comment.
Onboarding navigation is very confusing because:
- We use the same screens across different flows (e.g., create an account, sign in, profiles → create an account).
- Since we use the base screen as a modal for transparency, and then push screens using
navigate-to-within-stack, we have to track where we are coming from. Additionally, because we also usenavigate-to-within-stackin events, we end up polluting the app-db with flow-related data to keep track of this.
Solution: In onboarding, we push stacks inside the base modal. Wallet likely does something similar, with different logic for where to pop up (though I'm not sure — I need to check).
- We continue using
navigate-toalso for these flows, as we do in other areas. Navigation will push into the modal if it exists; otherwise, it will push into the base stack. - Do not change
navigate-to; instead, create a new event,navigate-within-base-modal.
In both cases, we will eliminate confusion and reduce noisy code, as seen in 336f1cb. (still WIP)
What do you think? Which solution sounds better, or do you have any suggestions?
Parveshdhull
commented
Nov 25, 2024
| @@ -43,4 +43,4 @@ | |||
| :stickers/packs-pending #{} | |||
| :settings/change-password {} | |||
| :keycard {} | |||
| :theme :light}) | |||
| :theme :dark}) | |||
There was a problem hiding this comment.
App opens in on-boarding/profiles, where default theme is :dark
Parveshdhull
force-pushed
the
fix/onboarding-usage-data-screen
branch
from
d62975f
to
7f754a1
Compare
Parveshdhull
commented
Nov 25, 2024
| @@ -277,7 +276,6 @@ | |||
|
|
|||
| (defn view | |||
| [] | |||
| (rn/use-mount #(rf/dispatch [:centralized-metrics/check-modal metrics-modal/view])) | |||
There was a problem hiding this comment.
For existing users who have not confirmed their choice, we will likely show the usage data screen after login. (Needs to discuss, cc @ilmotta)
Parveshdhull
requested review from
ilmotta and
flexsurfer
and removed request for
ilmotta
Parveshdhull
commented
Nov 25, 2024
| (defn- share-usage-data-fn | ||
| [enabled? next-screen] | ||
| (rf/dispatch [:centralized-metrics/toggle-centralized-metrics enabled? true]) | ||
| (rf/dispatch [:navigate-to next-screen])) |
There was a problem hiding this comment.
Used navigate-to instead of navigate-to-within-stack - https://github.com/status-im/status-mobile/pull/21655/files#r1856286497
0% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
|
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 #21563
Testing Note
status: ready for review (please postpone testing the PR until approved)