Bottom sheet with save and send options not displayed after scanning wallet QR using universal scanner #18928 #19250
Conversation
Jenkins BuildsClick to see older builds (79)
|
mmilad75
force-pushed
the
milad/18928-add-bottom-sheet-scan-qr-code
branch
from
bb80f0a
to
ff03943
Compare
mmilad75
changed the base branch from
develop
to
milad/19059-create-new-navigation-flow
| @@ -119,7 +122,12 @@ | |||
| (rn/dismiss-keyboard!)) | |||
| [scan-qr-code/view | |||
| {:title (i18n/label :t/scan-qr) | |||
| :on-success-scan on-qr-code-scanned}]])) | |||
| :on-success-scan on-qr-code-scanned}] | |||
| [quo/button | |||
There was a problem hiding this comment.
This button is just for testing and it will be removed in final push.
| (str short-name ":")])) | ||
| {:size size | ||
| :weight weight | ||
| :style {:color (colors/resolve-color (or network-name (keyword network)) theme)}} |
There was a problem hiding this comment.
btw (keyword network) looks suspicious since network is a map, no?
There was a problem hiding this comment.
No, the network is a string
(map #(colored-network-text {:theme theme :network % :weight weight :size size}))
There was a problem hiding this comment.
right, but you are also destructuring network up above
(let [{:keys [network-name short-name]} network
so then it's strange that it's a string or a map, probably shouldn't destructure the string in the case that's what it is 🤔
There was a problem hiding this comment.
I guess
(map #(colored-network-text {:theme theme :network {:network-name %} :weight weight :size size}))
is safer tbh 👍
There was a problem hiding this comment.
But you're destructing here which may be a string here also
|
|
||
| (defn view | ||
| [] | ||
| (let [on-close (fn [] |
There was a problem hiding this comment.
Just to be on the safe side, I would declare it outside the function body, so will not trigger any re-render
| (as-> networks $ | ||
| [{:keys [networks address blur? theme format full-address? size weight] | ||
| :or {size :paragraph-2}}] | ||
| (let [network-text-xf (map #(colored-network-text {:theme theme |
There was a problem hiding this comment.
that's quite a lot of work, maybe worth considering doing that in a dedicated sub, but don't have much context
There was a problem hiding this comment.
These codes were written by @ulisesmac and I just copied them
There was a problem hiding this comment.
I renamed to network-colored-text
There was a problem hiding this comment.
well, -xf is just the suffix for transducers, I see it as a -fn at the end of a var containing a function.
e.g.
it comes from xform 👀
src/quo/foundations/colors.cljs
Outdated
| @@ -245,8 +245,11 @@ | |||
| ;;;; Networks | |||
| (def ^:private networks | |||
| {:ethereum "#758EEB" | |||
| :eth "#758EEB" | |||
There was a problem hiding this comment.
I think that's fine, two maps is a bit neater, but definitely I would extract the colour if you want to go this way i.e:
(def ... networks
{:ethereum ethereum-colour
:eth ethereum-color
....
Though i have a slight preference to two maps and translate the key, but no strong opinion
There was a problem hiding this comment.
With help of @J-Son89 , we created a separate map
mmilad75
changed the title
mmilad75
requested review from
cammellos and
J-Son89
and removed request for
cammellos
mmilad75
requested review from
cammellos,
ilmotta,
vkjr,
briansztamfater,
smohamedjavid and
ulisesmac
96% of end-end tests have passedFailed tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@status-im/mobile-qa Can you please test this PR? |
Hi @mmilad75 Unfortunately, the entire quality assurance team is busy testing the release branch. I will do my best to check this PR as soon as we complete testing on the release. Thank you for your understanding. |
@VolodLytvynenko you can go ahaed and switch to PRs. We are stopping testing release until we figure out what todo with current blockers. |
|
Hi, @mmilad75, looks great! Thank you for the PR. Please take a look at one issue I found, which could be considered as a follow-up. If better to fix it separately, then PR can be merged ISSUE 1: From page is missed after Wallet QR was scanned via opened accountSteps:
Actual result:The 'from' screen is missed from.mp4Expected result:The from the list should be shown after the 'send' option is tapped |
fixes #18928
** Note that the buttons in the qr scanner screen is just for demo purposes and not included in the codes
scan_address.mp4