Update UI for Sign In by Syncing and Add Scan or Enter pairing Code to in app syncing flow #15623
Conversation
Jenkins BuildsClick to see older builds (48)
|
J-Son89
force-pushed
the
jc/sync-auth-3
branch
2 times, most recently
from
6c16b04
to
fbe11a5
Compare
J-Son89
force-pushed
the
jc/sync-auth-3
branch
5 times, most recently
from
0aec045
to
703967c
Compare
J-Son89
changed the title
J-Son89
requested review from
smohamedjavid,
Parveshdhull,
ilmotta,
briansztamfater,
jo-mut and
cammellos
| :camera-options {:zoomMode :off} | ||
| :scan-barcode true | ||
| :on-read-code on-read-code}]] | ||
| [rn/view {:width size :height size :justify-content :space-between} |
There was a problem hiding this comment.
should set styles in :style prop
| :text (i18n/label | ||
| :t/camera-permission-denied)}])}]))] | ||
| [:f> | ||
| (fn [] |
There was a problem hiding this comment.
(fn [] should be never used in the hiccup. for proper usage, you have to rename view to f-view and used it with [:f>
| (defn- border | ||
| [border1 border2 corner] | ||
| [rn/view | ||
| (assoc {:border-color colors/white :width 80 :height 80} border1 2 border2 2 corner 16)]) |
There was a problem hiding this comment.
@J-Son89 - The PR looks good!
I have one question regarding the behaviour of the Camera.
The camera renders within the viewfinder which is different to current UI behaviour of rendering the camera in fullscreen behind the blur. Is this a expected?
| Existing behaviour | New change |
|---|---|
 |
 |
If we don't need to render the camera fullscreen, we can remove the holeview usage and qr-view-finder calculation.
Thanks @smohamedjavid, you are right. However I think both of these versions are incorrect. However in both current and this pr version when the camera is enabled it is a bit different in the designs. |
| {:borderRadius 16})]} | ||
| background] | ||
| background) | ||
| ) |
There was a problem hiding this comment.
The dreaded dangling parenthesis attacks again!
| [rn/view {:style (style/root-container (:top insets))} | ||
| [header active-tab read-qr-once? title] | ||
| (case @active-tab | ||
| 1 [scan-qr-code-tab qr-view-finder request-camera-permission |
There was a problem hiding this comment.
I think a bit better than 1 or 2 is to give meaningful names to the tabs, like :tab/scan-qr-code and :tab/enter-sync-code.
There was a problem hiding this comment.
Yep that's a good idea 👍
| (defn- viewfinder | ||
| [qr-view-finder {:keys [camera-ref on-read-code]}] | ||
| (let [size (:width qr-view-finder)] | ||
| [:<> |
There was a problem hiding this comment.
This fragment :<> seems unnecessary.
J-Son89
force-pushed
the
jc/sync-auth-3
branch
2 times, most recently
from
a176c52
to
02de8c2
Compare
87% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (27)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
hi @J-Son89 . Thank you for PR. No issues from my side. PR is ready to be merged |
thanks @VladimrLitvinenko! :) |
The Onboarding Syncing flow and In app syncing flow is the same except for the background.
This pr refactors the onboarding syncing screens so that it can be reused for both onboarding and in app syncing flows.
Also I adjust the onboarding syncing page as the background was not showing correctly.
To test:
Ensure the onboarding Syncing flows are working as expect. (i.e "sign in")
Also you can test the syncing flows by going
profile -> Syncing -> big blue "+" button and then "Scan or enter pairing code" button at the bottom of the "Pair devices to sync" page.