scan watched address #17829
Merged
scan watched address #17829
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 (51)
|
J-Son89
force-pushed
the
jc/scan-watched-address
branch
from
df9c2ee
to
e6ff5fb
Compare
J-Son89
commented
Nov 8, 2023
| @@ -1,4 +1,4 @@ | |||
| (ns status-im2.contexts.wallet.add-watch-only-account.views | |||
| (ns status-im2.contexts.wallet.add-watch-only-account.view | |||
There was a problem hiding this comment.
aligning with best practices
J-Son89
commented
Nov 8, 2023
| @@ -36,7 +36,7 @@ | |||
| {:customization-color account-color | |||
| :size 80 | |||
| :emoji account-emoji | |||
| :type :default}] | |||
| :type (if watch-only? :watch-only :default)}] | |||
There was a problem hiding this comment.
this is the fix for: #17807
J-Son89
commented
Nov 8, 2023
| [utils.i18n :as i18n] | ||
| [utils.re-frame :as rf])) | ||
|
|
||
| (defn view-internal | ||
| (defn- address-input | ||
| [input-value input-focused?] |
There was a problem hiding this comment.
similar code to send scan address, most pieces here are reusable so I just copied the relevant pieces over to avoid coupling the send and watch only scanners.
J-Son89
commented
Nov 8, 2023
| [] | ||
| (let [top (safe-area/get-top) |
There was a problem hiding this comment.
there was excess margin-top here
J-Son89
commented
Nov 8, 2023
| @@ -248,8 +248,8 @@ | |||
| :options {:insets {:top? true}} | |||
| :component wallet-accounts/view} | |||
|
|
|||
| {:name :address-to-watch-edit | |||
There was a problem hiding this comment.
I found "edit" to be a bit misleading. We have "edit" account and so I did not want to confuse with that. here we are creating (or adding) the watch only account.
There was a problem hiding this comment.
I am working on an issue #17806 related to address watch, and I was also doing changes to the naming :) I pushed my PR to show the changes I made to avoid conflicts: https://github.com/status-im/status-mobile/pull/17859/files
I renamed select-address-to-watch to add-address-to-watch (to match the screen title) and moved add-watch-only-account under add-address-to-watch namespace and renamed it to edit-address (and screen name edit-addres-to-watch). I think that makes it clearer that the editing is for the watch address.
Also, we can use the word confirm instead of edit
There was a problem hiding this comment.
Sure I'll copy your changes, thanks for the heads up! 🙂
There was a problem hiding this comment.
Renamed it to confirm-address. And PR is ready for review #17859 👍
J-Son89
requested review from
OmarBasem,
tumanov-alex,
smohamedjavid,
mmilad75,
vkjr,
briansztamfater and
ulisesmac
| @@ -36,7 +36,7 @@ | |||
| {:customization-color account-color | |||
| :size 80 | |||
| :emoji account-emoji | |||
| :type :default}] | |||
| :type (if watch-only? :watch-only :default)}] | |||
| (defn view-internal | ||
| (defn- address-input | ||
| [input-value input-focused?] | ||
| (fn [] |
There was a problem hiding this comment.
I guess we can remove this (fn []) as we don't have any atoms.
There was a problem hiding this comment.
sure I'll take a look at removing that.
There was a problem hiding this comment.
Yes, that's true, the fn [] is not needed.
Actually, this (fn[]) will cause problems with rerenders.
If input-value or input-focused? change, the body of the function will not be re executed, and if the component rerenders (e.g. because a sub changes), input-value and input-focused will have the value they had when the component mounted, not the current one
| input-value (reagent/atom "") | ||
| customization-color (rf/sub [:profile/customization-color])] | ||
| input-focused? (reagent/atom false)] |
There was a problem hiding this comment.
I noticed that the input-focused? value is not used inside the address-input. Are we planning to use it in future?
There was a problem hiding this comment.
whoops, I was being a bit blind there 😓 I'm going to remove this, and actually @briansztamfater I think on the send address screens we eventually should be using this as it will (should) handle that behaviour
#17737
There was a problem hiding this comment.
will use the same approach on this page when that pr is merged too 👍
| (fn [] | ||
| (rn/use-effect (fn [] #(rf/dispatch [:wallet/clean-scanned-address]))) |
There was a problem hiding this comment.
I believe we can dispatch this clean-up event (before the let) prior to UI mount without the use-effect (and the functional component [:f>] wrapping). Just like we did on the wallet home page for collectables.
There was a problem hiding this comment.
sure, I'll do that 👍
ulisesmac
approved these changes
Nov 8, 2023
src/status_im2/contexts/wallet/select_address_to_watch/view.cljs
Outdated
Show resolved
Hide resolved
| (defn view-internal | ||
| (defn- address-input | ||
| [input-value input-focused?] | ||
| (fn [] |
There was a problem hiding this comment.
Yes, that's true, the fn [] is not needed.
Actually, this (fn[]) will cause problems with rerenders.
If input-value or input-focused? change, the body of the function will not be re executed, and if the component rerenders (e.g. because a sub changes), input-value and input-focused will have the value they had when the component mounted, not the current one
J-Son89
force-pushed
the
jc/scan-watched-address
branch
2 times, most recently
from
9943f31
to
8a7a14c
Compare
smohamedjavid
approved these changes
Nov 9, 2023
|
|
||
| (defn view | ||
| [] | ||
| [:f> f-view]) |
There was a problem hiding this comment.
We can move all code from the f-view to view and remove the [:f>] as we don't use any react hooks inside this page.
briansztamfater
approved these changes
Nov 9, 2023
Comment on lines
49
to
50
| [rn/view | ||
| {:style {:flex 1}} |
There was a problem hiding this comment.
Super minor, this can be in the same line
Np, was easy to reuse the code you added for this 💪🏼 |
OmarBasem
approved these changes
Nov 10, 2023
J-Son89
force-pushed
the
jc/scan-watched-address
branch
2 times, most recently
from
74a472f
to
77f0861
Compare
|
Hi @J-Son89 thank you for the fixes. Actual result:
Expected result:How it shown on design and latest nightly: |
ISSUE 8: Incorrect address added to ETH address textbox after scanning QR CodeSteps:
Actual result:The invalid address is added into ETH textbox video.mp4Expected result:The valid address contained in the QR should be added into the ETH textbox. Device:Pixel 7a, Android 13 Logs:Additional info:Currently, the issue reproduces randomly, occurring only when quickly initiating the camera hover from the QR's edge |
|
@J-Son89 But it's alright if this issue gets fixed separately. It might not be within the scope of this PR. Please inform me if it will be addressed in this PR. ISSUE 9: Random color picker selected after tapping 'Continue' during watch only address creationSteps:
Actual result:The random color picker is selected color_picker.mp4Expected result:The default selection should be the dark blue (first color picker). |
|
Thank @VolodLytvynenko - I will address these. Actually the random color is expected behaviour. The only account which needs to to be same as the user color is the initial account but this can be adjusted afterwards. The initial account emoji is also random. I'll ask designers to add a note in the figma file with this! |
That's correct! 👍 |
J-Son89
force-pushed
the
jc/scan-watched-address
branch
from
9cb7a6b
to
89fa43c
Compare
Fixed now, and verified on ios build alongside figma designs 👍 |
42% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (19)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
84% of end-end tests have passedFailed tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (38)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
This was referenced Nov 19, 2023
The 'ENS or chat key' input field does not adjust its size to the scanned data from a QR code
#17943
Closed
|
Hi @J-Son89 Thank you for fixes. No issues from my side. PR is ready to be merged
|
VolodLytvynenko
added
Tested - OK
test cases required
and removed
Tested - Issues
labels
J-Son89
force-pushed
the
jc/scan-watched-address
branch
from
cddd7b5
to
e181d62
Compare
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 #17808
fixes #17807
This pr fixes two issues.
allow user to scan address on watch only flows
adjust background color of emoji picker (account avatar) on create watch only account page ->
Also I adjusted copy from "watch address" subtitle to "Watched address" as pointed out in the demo by designers 👍
Test Notes:
make sure to create a new user when testing.
this does not add the ability to create a watch only flow - that will be added later.
trim.B55E575A-CC47-45BB-B5A8-F256556EC7CA.MOV