-
Notifications
You must be signed in to change notification settings - Fork 986
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
Fix syncing flow navigation and designs #21884
Conversation
Jenkins Builds
|
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.
Based on your perceptions #21765 (comment), it would be good to take the opportunity from this PR and issue to see if the biometrics screen should be re-added or not to the onboarding after syncing is complete. Maybe it was a mistake in Figma. I think we should show the biometrics step in order to be more consistent with what other apps do after initial account set-up, but we better hear from designers as well. Not a blocker for this PR 👍🏼
Thank you @Parveshdhull
38% of end-end tests have passed
Failed tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
Thank you @ilmotta
Added a comment/question in figma - https://www.figma.com/design/o4qG1bnFyuyFOvHQVGgeFY?node-id=15071-95173&m=dev#1081016843 |
Thank you @Parveshdhull! Alisher has replied in Figma: @Parveshdhull based on that, would you mind re-add biometric screen in this PR or you prefer to do it separately? |
Thank you very much @pavloburykh for taking the PR for testing. I will re-add the bio-metric screen in the same PR. |
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.
Thank you for this PR @Parveshdhull !
:blur? true | ||
:button-one-label (i18n/label :t/recovery-phrase) | ||
:button-one-props {:type :primary | ||
:container-style {:height (when-not logged-in? 116)} |
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.
Can we give a name to this 116?
I'm wondering what does it mean, I guess it's a sum of different UI elements because it's a big number. I also wonder if it's considering how it looks on different platforms.
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.
Thank you @ulisesmac for reviewing the PR. I have added a label here - https://github.com/status-im/status-mobile/pull/21890/files#diff-d6d12a107e3ffe116d992ea055af107e6b7c68f5f3084958999d739b01f15dfe
Hi @pavloburykh, @ilmotta I was hoping that re-adding the biometric screen would only require a small change, but it seems that’s not the case.
To achieve this, I had to make several changes, which will require another review. I’ve created a separate PR for this I will finish that PR once this one is merged. Pavlo, please go ahead with testing. Thank you! |
@Parveshdhull thanks you!
I think we should better confirm with @xAlisher if this has been made intentionally. I have some doubts based on the fact, that in case of Sync fallback https://www.figma.com/design/o4qG1bnFyuyFOvHQVGgeFY/Onboarding-for-Mobile?node-id=15644-50720&m=dev we still have another Enable biometric step. And this step does not make sense since we have already enable/disabled biometrics right after scanning QR code. So, we either need to remove Biometric step from Sync fallback flow or move Enable biometric step (in standard Sync flow) after syncing is complete screen like it was before. |
@Parveshdhull I was also expecting the biometrics step to be the final step as we discussed in this PR, i.e. only after syncing is complete. Worth re-checking as @pavloburykh suggested. |
@Parveshdhull based on discussion with @xAlisher biometric step has been moved after syncing is complete (as it was before) https://www.figma.com/design/o4qG1bnFyuyFOvHQVGgeFY/Onboarding-for-Mobile?node-id=16409-27876&m=dev Please tell, if you still prefer to make those changes in separate PR or now it is okay to implement here. Sorry for initial confusion. |
Thank you very much @pavloburykh for getting this resolved. Let's handle this leftover in a separate PR. That PR introduces a few other changes and refactors that I want to keep. |
Thanks @Parveshdhull This PR is ready for merge. |
c42c019
to
9281eac
Compare
fixes #21765
Video
signal-2025-01-03-143029.mp4
status: ready