add navigate biometric screen after sign-in by syncing #17627
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
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
from
0b4cbb1 to
21452f2
Compare
jo-mut
changed the title
Jenkins BuildsClick to see older builds (109)
|
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
7 times, most recently
from
45682ce to
019c6e5
Compare
jo-mut
requested review from
J-Son89,
ajayesivan,
smohamedjavid,
ibrkhalil and
siddarthkay
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
2 times, most recently
from
2efab21 to
2260b1f
Compare
ajayesivan
reviewed
Oct 27, 2023
|
@ajayesivan I thought adding the |
|
@ajayesivan thanks for pointing that out. it works well on android but looks like on ios there is still something to do to make it work on app relaunch. Am working on it |
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
2 times, most recently
from
86977dd to
f8c7a08
Compare
ibrkhalil
approved these changes
Oct 30, 2023
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
4 times, most recently
from
dbae1fe to
dada4e4
Compare
|
@ajayesivan On relaunch biometrics works well now |
@jo-mut the issue is reproducible in the builds with commit dea9b0d These were the latest available builds when I was testing this PR. 
I have just tried the latest builds after your recent rebasing and I am not reproducing ISSUE 2 anymore.
@jo-mut is the design team aware about this technical limitations and current solution confirmed with them? Asking because I do not see any updates in Figma. I believe that whenever we need to update the flow due to any technical reason we should bring this to designers as well so they confirm solution from their perspective. Meanwhile, I think that name of the button should be changed. Instead of "Password" we need something which describes the action like Confirm/Proceed/Next etc.
|
|
@pavloburykh I think |
J-Son89
reviewed
Nov 14, 2023
| @@ -35,11 +35,30 @@ | |||
| {:biometric/authenticate {:on-success #(rf/dispatch [:onboarding-2/biometrics-done]) | |||
| :on-fail #(rf/dispatch [:onboarding-2/biometrics-fail %])}}) | |||
|
|
|||
| (rf/defn authenticate-enable-biometrics | |||
There was a problem hiding this comment.
@jo-mut - I think there is a slightly better approach to take here. I will make a suggested commit and you can let me know what you think 👍
There was a problem hiding this comment.
please see 1392a4e - I have this code pushed on branch jc/standard-auth-refactor
The problem here is that the standard auth code only exposes a slide button, so you had to make these adjustments for it to work with the quo/button.
The better approach imo is to add another component in standard-auth "module" - which uses quo/button component under the hood, we can then encapsulate all of this authorization logic in there 👍
I think we will need for many other screens anyway so it would be good to have this added in 👍
There was a problem hiding this comment.
I made a slight adjustment - 1ad8626 (also updated requires in wallet screens 👍 )
There was a problem hiding this comment.
@J-Son89 I have looked at your branch. I agree with your suggestions. thanks
|
Design input required @status-im/design-team . Link to the discussion |
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
2 times, most recently
from
13bbf97 to
c3027ed
Compare
49% of end-end tests have passedFailed tests (21)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (22)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
@J-Son89 could you take a look at this pr once again. I included your suggestions here #17627 (comment). Am just wondering so e2e test have a pass rate that is so low |
We are experiencing global failing of e2e right now. Currently investigating the reason. |
44% of end-end tests have passedFailed tests (22)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (20)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
18% of end-end tests have passedFailed tests (18)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Passed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
|
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
from
c3027ed to
570a43f
Compare
73% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (33)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@J-Son89 @ajayesivan may I ask you to confirm that your review comments were addressed? |
J-Son89
approved these changes
Nov 17, 2023
|
@J-Son89 thank you |
|
@jo-mut please, cherry-pick your commit to release branch, thanks! |
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
from
570a43f to
54a0e6c
Compare
jo-mut
force-pushed
the
fix/enable-biometrics-screen
branch
from
54a0e6c to
3e4ef18
Compare
cammellos
pushed a commit
that referenced
this pull request
Nov 17, 2023
yevh-berdnyk
pushed a commit
that referenced
this pull request
Dec 8, 2023
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 #15643
Summary
This pr adds
enable biometrics screenin the sign-in flow.Review notes
It does not include the
preparing status for youscreen in the following figma link belowhttps://www.figma.com/file/o4qG1bnFyuyFOvHQVGgeFY/Onboarding-for-Mobile?type=design&node-id=3515-358293&mode=design&t=shfNKJEPbKyMj2cx-4
Platforms
Steps to test
status: ready