Skip to content
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.

Sign up for GitHub

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

[#18934] universal scanner in wallet receive #19409

Merged
merged 5 commits into from
Mar 27, 2024
Merged

[#18934] universal scanner in wallet receive #19409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fa5da2a
Fix linear-gradient breaking when `customization-color` is unknown
ulisesmac Mar 26, 2024
b3a6b4b
Fix exception while scanning an address due to non-existing navigatio…
ulisesmac Mar 26, 2024
91efaa8
Small refactor
ulisesmac Mar 26, 2024
f33d3b8
Dispatches universal QR code scanner on share address modal
ulisesmac Mar 26, 2024
b68e76d
Fix screen keyword
ulisesmac Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/quo/components/gradient/gradient_cover/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
;; the `:or` destructuring won't work because it's only applied when the
;; `:customization-color` key is non-existent. While deleting an account the key exists
;; and has a `nil` value.
(when customization-color
(let [color-top (colors/resolve-color customization-color 50 20)
color-bottom (colors/resolve-color customization-color 50 0)]
(let [color-top (colors/resolve-color customization-color 50 20)
color-bottom (colors/resolve-color customization-color 50 0)]
(when (and color-top color-bottom)
[linear-gradient/linear-gradient
{:accessibility-label :gradient-cover
:colors [color-top color-bottom]
Expand Down
27 changes: 12 additions & 15 deletions src/quo/components/wallet/account_overview/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,7 @@
:or {customization-color :blue}}]
(let [time-frame-string (time-string time-frame time-frame-string)
up? (= metrics :positive)]
[rn/view
{:style style/account-overview-wrapper}
[rn/view {:style style/account-overview-wrapper}
(if (= :loading state)
[loading-state (colors/theme-colors colors/neutral-5 colors/neutral-90 theme)]
[rn/view
Expand All @@ -113,19 +112,17 @@
:size :heading-1
:style style/current-value}
current-value]
[rn/view
{:style style/row-centered}
[:<>
(when (seq time-frame-string)
[text/text
{:weight :medium
:size :paragraph-2
:style (style/bottom-time-text (and (not= :custom time-frame)
(seq time-frame-to-string)))}
time-frame-string])
(when (and (= :custom time-frame)
(seq time-frame-to-string))
[custom-time-frame time-frame-to-string])]
[rn/view {:style style/row-centered}
(when (seq time-frame-string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to use string/blank? to check for string emptiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just refactored this code to remove the unnecessary :<> there.

But I agree, I'll update it, to improve the whole piece of code 👍 thanks for spotting it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ibrkhalil

I checked the implementation, and I'd prefer keep it as is since it's a bit complex, it's not just about checking a string.

the component receives:
time-frame, time-frame-string and time-frame-to-string.

then it shadows
time-frame-string with (time-string time-frame time-frame-string)

and sometimes time-frame-string will take the value of the received time-frame-string, so I guess the decision to use seq was because it's more general.

Since it's a bit hard to track all the flow to make sure this won't create a bug in some cases, I'd prefer to keep it as is, additionally, the seq is being used in a translation string, so it won't be "" or " ".

[text/text
{:weight :medium
:size :paragraph-2
:style (style/bottom-time-text (and (not= :custom time-frame)
(seq time-frame-to-string)))}
time-frame-string])
(when (and (= :custom time-frame)
(seq time-frame-to-string))
ulisesmac marked this conversation as resolved.
Show resolved Hide resolved
[custom-time-frame time-frame-to-string])
(when (and (seq percentage-change)
(seq currency-change))
[numeric-changes percentage-change currency-change customization-color theme up?])]])]))
Expand Down
2 changes: 1 addition & 1 deletion src/status_im/contexts/shell/qr_reader/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
(load-and-show-profile scanned-text)

(eth-address? scanned-text)
(debounce/debounce-and-dispatch [:navigate-to :wallet-accounts scanned-text] 300)
(debounce/debounce-and-dispatch [:navigate-to :screen/wallet.accounts scanned-text] 300)

(eip681-address? scanned-text)
(do
Expand Down
4 changes: 3 additions & 1 deletion src/status_im/contexts/wallet/share_address/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@
:on-press #(rf/dispatch [:navigate-back])
:background :blur
:right-side [{:icon-name :i/scan
:on-press #(js/alert "To be implemented")}]
:on-press (fn []
(rf/dispatch [:navigate-back])
(rf/dispatch [:open-modal :shell-qr-reader]))}]
:accessibility-label :top-bar}]
[quo/page-top
{:title title
Expand Down