-
Notifications
You must be signed in to change notification settings - Fork 985
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
onboarding syncing updates #16206
onboarding syncing updates #16206
Conversation
Jenkins BuildsClick to see older builds (62)
|
b04a694
to
dd467a1
Compare
fbd7ae7
to
dd5a101
Compare
dd5a101
to
ceaf82d
Compare
Hi @jo-mut I was trying to review and contrast the changes, but it's being a little hard to know what exactly expect. btw, this PR has the "fixes #..." but there're two remaining tasks in the issue, so I'd suggest to open new issues for those tasks or not closing this issue with this PR. wdyt? Thanks again. |
ceaf82d
to
b97c208
Compare
@ulisesmac I have added some screenshots to the p.r. As you might have seen some of the issues are not addressed in this pr. They can be addressed in a separate issue if the Q.A agrees with that and once the Q.A is okay with this pr it would then be fine to open a new issue. There seems to be an issue with the camerakit as some of the camera features dont seems to work on both android and ios which is why they will better addressed in a separate issue as they have to do with the camerakit library we are using |
Thank you! |
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.
Thanks for the PR! The screen has been improved! 💯
:border-style :dashed | ||
:border-width 1 |
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.
How this change is showed?
I think this def
(camera-permission-container
) is not being used 🤔 inspecting a little more the code in this file, it seems like many def
s are not being used, probably it's unrelated, but would you mind removing these unused vars if you are going to touch this file?
thanks!! :)
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.
I guess I knew the answer to it.
A refactor happened last month (#15623) where the sign-in code has been moved to a common component (scan-sync-code
) for making it easy to use from "Sign In" and "Add Device/Scan Sync Code" screens. But, the styles of sign-in haven't been removed. We can delete it.
[rn/view | ||
{:style (style/qr-view-finder-container size)} | ||
[rn/view | ||
{:flex-direction :row :justify-content :space-between} |
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.
Please add these styles inside a :style
key and move them to the style namespace
:type :blur-bg | ||
:size 32 | ||
:accessibility-label :camera-flash | ||
:on-press #() |
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.
If we are not going to use the on-press
call back we could remove the key from the map, I think that's better than providing an empty function
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.
Nice work! 🚀
:border-style :dashed | ||
:border-width 1 |
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.
I guess I knew the answer to it.
A refactor happened last month (#15623) where the sign-in code has been moved to a common component (scan-sync-code
) for making it easy to use from "Sign In" and "Add Device/Scan Sync Code" screens. But, the styles of sign-in haven't been removed. We can delete it.
@@ -142,21 +143,63 @@ | |||
(defn- border | |||
[border1 border2 corner] | |||
[rn/view | |||
(assoc {:border-color colors/white :width 80 :height 80} border1 2 border2 2 corner 16)]) | |||
(assoc style/border border1 2 border2 2 corner 16)]) |
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.
Kindly move these styles as value to :style
key.
781c7c3
to
b0609c1
Compare
@VladimrLitvinenko thank your observations. These issues are now resolved. |
5d0467a
to
758761c
Compare
76% of end-end tests have passed
Failed tests (8)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (25)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
758761c
to
4ab5422
Compare
100% of end-end tests have passed
Passed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
4ab5422
to
abaf01d
Compare
@VladimrLitvinenko the issue addressed in this pr has three bugs that are not yet resolved
|
abaf01d
to
dbed7e8
Compare
@VladimrLitvinenko no the current pr is fully done. I mean to say the remaining add flash to camera will be addressed in another pr. Just stating for the record but this is okay to merge |
Got it. Thank you for the clarification |
dbed7e8
to
f413502
Compare
f413502
to
0795f91
Compare
fixes #15787
Summary
Resolves the issues pointed out except illustrations and support camera flash functionality. Seems the camerakit library has some issues with flash as it does not seem to work on both android and ios