Refactor add address to watch component and make it generic and use it in add saved addresses screen behind a FF #19797
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
ibrkhalil
changed the title
Jenkins BuildsClick to see older builds (140)
|
ibrkhalil
changed the title
cammellos
reviewed
Apr 25, 2024
| @@ -36,6 +38,10 @@ | |||
| {:title (i18n/label :t/saved-addresses) | |||
| :accessibility-label :saved-addresses-header | |||
| :right :action | |||
| :on-press (fn [] | |||
There was a problem hiding this comment.
you should probably declare this outside, you can check if it triggers a re-render of the overlay if the sub wallet/saved-addresses changes (I believe it will, but if you declare it outside, it should not)
| @@ -21,8 +22,9 @@ | |||
| [] | |||
| (let [inset-top (safe-area/get-top) | |||
| customization-color (rf/sub [:profile/customization-color]) | |||
| saved-addresses [] | |||
| saved-addresses (rf/sub [:wallet/saved-addresses]) | |||
There was a problem hiding this comment.
I take the plan is to actually use saved addresses eventually (in that case this is fine for the time being of course), but if you only are checking whether there's any saved addresses, you should subscribe only to (seq saved-addresses), so that the view re-renders only when you flip from 0 to 1 or viceversa (maybe I am missing where you use it though, but couldn't find it)
There was a problem hiding this comment.
actually (boolean (seq saved-addresses)) should be the subscription, (seq saved-addresses) is not good enough
| placeholder (-> confirming-addresses-purposes | ||
| (get-in [purpose :placeholder]) | ||
| i18n/label) | ||
| account-name (reagent/atom "") |
| :bottom-action-props {:customization-color @account-color | ||
| :disabled? (string/blank? @account-name) | ||
| :accessibility-label :confirm-button-label | ||
| :on-press (fn [] |
There was a problem hiding this comment.
maybe this function could be defined outside? it's rather large for an on-press handler
| :weight :monospace} | ||
| address]) | ||
| :container-style style/data-item | ||
| :on-press #(js/alert "To be implemented")}]]]]))) |
There was a problem hiding this comment.
you could define it outside, to show the right pattern, even if it's not implemented :)
| (cond | ||
| (or (nil? user-input) (= user-input "")) nil | ||
| (contains? known-addresses user-input) (i18n/label :t/address-already-in-use) | ||
| (or (nil? user-input) (= user-input "")) nil |
There was a problem hiding this comment.
empty? doesn't cater for whitespace, I think it shouldn't be used for strings often
There was a problem hiding this comment.
you can use blank? of course, but that would be changing the functionality, not sure what the original intention of the code was, empty? should retain it, while blank adds some extra cases, but either are fine by me, but or looks unneccessary
| (rf/dispatch [:wallet/clean-scanned-address])) | ||
| customization-color (rf/sub [:profile/customization-color])] | ||
| (let [addresses (rf/sub [:wallet/addresses]) | ||
| lowercased-addresses (map string/lower-case addresses) |
There was a problem hiding this comment.
looks like probably you want this int the sub since you are not using addresses?
| customization-color (rf/sub [:profile/customization-color])] | ||
| (let [addresses (rf/sub [:wallet/addresses]) | ||
| lowercased-addresses (map string/lower-case addresses) | ||
| input-value (reagent/atom nil) |
J-Son89
reviewed
Apr 25, 2024
|
|
||
| (def save-address-drawer-bar-container | ||
| {:position :absolute | ||
| :left "50%" |
There was a problem hiding this comment.
We shouldn't need percentages here, probably can be done without
|
|
||
| (defn view | ||
| [] | ||
| (let [{:keys [address purpose ens?]} (rf/sub [:get-screen-params]) |
There was a problem hiding this comment.
'Purpose' here really lacks clarity of the use. It's very generic, is there something more informative we can use?
There was a problem hiding this comment.
To me it also suggests that we would be better to abstract some pieces of this page but not the whole page, or at least have it configurable from outside rather than encapsulating these properties.
Wdyt @briansztamfater, it was a topic we talked about yesterday
J-Son89
reviewed
Apr 25, 2024
| (defn- validate-address | ||
| [known-addresses user-input] | ||
| [known-addresses user-input purpose] |
There was a problem hiding this comment.
Imo Better to pass the text (i18n label) than this generic purpose which is rather specific here
ibrkhalil
force-pushed
the
refactor-add-addresses-to-watch-screen/logic
branch
from
9cd5f68
to
f2f2b97
Compare
cammellos
reviewed
Apr 26, 2024
| @@ -17,12 +18,26 @@ | |||
| :image (resources/get-themed-image :sweating-man theme) | |||
| :container-style style/empty-container-style}])) | |||
|
|
|||
| (defn on-press | |||
cammellos
reviewed
Apr 26, 2024
| :account-emoji account-emoji | ||
| :account-color account-color | ||
| :address address})}} | ||
| (prn "meg " button-label) |
| (rf/reg-sub | ||
| :wallet/addresses | ||
| :<- [:wallet] | ||
| :-> #(->> % | ||
| :accounts | ||
| keys | ||
| to-lower-case |
There was a problem hiding this comment.
maybe you can create a separate subscription for this? wallet/lowercase-addresses based on this one, so we leverage caching
|
Drafting till improvements are done. |
|
@ibrkhalil - since this pr is in draft now I'd suggest removing the |
ibrkhalil
force-pushed
the
refactor-add-addresses-to-watch-screen/logic
branch
2 times, most recently
from
d55e54c
to
7fe3fa6
Compare
ibrkhalil
changed the title
ibrkhalil
requested review from
flexsurfer
and removed request for
shivekkhurana,
smohamedjavid and
ulisesmac
ibrkhalil
force-pushed
the
refactor-add-addresses-to-watch-screen/logic
branch
from
abeefd0
to
a2766eb
Compare
seanstrom
reviewed
May 6, 2024
src/status_im/contexts/wallet/add_account/add_address/confirm_address/view.cljs
Outdated
Show resolved
Hide resolved
ibrkhalil
force-pushed
the
refactor-add-addresses-to-watch-screen/logic
branch
from
f7c439f
to
258b1ec
Compare
ibrkhalil
commented
May 13, 2024
| @@ -56,23 +56,35 @@ | |||
|
|
|||
|
|
|||
| (defn root-container | |||
| [{:keys [type size customization-color] | |||
| [{:keys [type size customization-color emoji?] | |||
There was a problem hiding this comment.
Adding text only (initials) styling to the emoji styling.
Comment on lines
+81
to
+85
| :border-radius width | ||
| :align-items :center | ||
| :justify-content :center | ||
| :color (colors/resolve-color customization-color theme 60) | ||
| :background-color (colors/resolve-color customization-color theme 20)}))) |
There was a problem hiding this comment.
Main addition is border-radius, color and background-color
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| (h/test "with emoji" | |||
| (let [emoji "💸"] | |||
| (h/render [account-avatar/view {:emoji emoji :size 80}]) | |||
| (h/render [account-avatar/view {:emoji emoji :size 80 :emoji? :true}]) | |||
Comment on lines
+21
to
+43
| [{:keys [size emoji account-name-initials emoji?] | ||
| :or {size style/default-size | ||
| emoji "🍑" | ||
| emoji? true} | ||
| :as opts}] | ||
| (let [theme (quo.theme/use-theme) | ||
| emoji-size (style/get-emoji-size size)] | ||
| [rn/view | ||
| {:accessible true | ||
| :accessibility-label :account-avatar | ||
| :style (style/root-container opts theme)} | ||
| [rn/text | ||
| {:accessibility-label :account-emoji | ||
| :adjusts-font-size-to-fit true | ||
| :style {:font-size emoji-size}} | ||
| (when emoji (string/trim emoji))]])) | ||
| (if emoji? | ||
| [rn/text | ||
| {:accessibility-label :account-emoji | ||
| :adjusts-font-size-to-fit true | ||
| :style {:font-size emoji-size}} | ||
| (when emoji (string/trim emoji))] | ||
| [text/text | ||
| {:accessibility-label :account-name | ||
| :size :heading-1 | ||
| :weight :medium | ||
| :style (style/account-name opts)} | ||
| account-name-initials])])) |
There was a problem hiding this comment.
Add support for initials as saved addresses don't have emojis.
Comment on lines
-37
to
-38
| ;; TODO: monospace font | ||
| ;; https://github.com/status-im/status-mobile/issues/17009 |
There was a problem hiding this comment.
Deleting a duplicated comment
| @@ -0,0 +1,12 @@ | |||
| (ns status-im.contexts.wallet.common.address-input.style) | |||
| @@ -0,0 +1,63 @@ | |||
| (ns status-im.contexts.wallet.common.address-input.view | |||
Comment on lines
+30
to
+37
| account-name-initials (when-not emoji? | ||
| (->> (string/split (if (string/blank? account-name) | ||
| placeholder | ||
| account-name) | ||
| #" ") | ||
| (map (fn [segment] | ||
| (utils.string/get-initials segment 1))) | ||
| (apply str)))] |
There was a problem hiding this comment.
Maybe we have a helper that extracts initials from multiple whitespace-separated words?
|
|
||
| (defn view | ||
| [{:keys [page-nav-right-side placeholder account-name account-color account-emoji | ||
| on-change-name | ||
| on-change-color | ||
| on-change-emoji section-label | ||
| bottom-action-label bottom-action-props | ||
| custom-bottom-action watch-only?]} & children] | ||
| custom-bottom-action watch-only? top-left-icon]} & children] |
There was a problem hiding this comment.
Added top-left-icon to customize the exit icon.
| @@ -11,7 +11,7 @@ | |||
| [utils.re-frame :as rf])) | |||
|
|
|||
| (defn view | |||
| [{:keys [selected-networks account watch-only?]}] | |||
| [{:keys [selected-networks account watch-only? sheet-title sheet-description]}] | |||
seanstrom
reviewed
May 13, 2024
Comment on lines
+21
to
+24
| [{:keys [size emoji account-name-initials emoji?] | ||
| :or {size style/default-size | ||
| emoji "🍑" | ||
| emoji? true} |
There was a problem hiding this comment.
Small tweak 🔧
Do we need to pass emoji? as a prop?
Could we compute emoji? based on whether the emoji field is present (not nil)?
Comment on lines
+39
to
+43
| {:accessibility-label :account-name | ||
| :size :heading-1 | ||
| :weight :medium | ||
| :style (style/account-name opts)} | ||
| account-name-initials])])) |
There was a problem hiding this comment.
tiny suggestion 🔖
maybe the :accessibility-label should be :account-initials or :account-name-initials?
Comment on lines
+35
to
+40
| [input-value set-input-value] (rn/use-state nil) | ||
| [validation-msg set-validation-message] (rn/use-state nil) | ||
| clear-input (fn [] | ||
| (set-input-value nil) | ||
| (set-validation-message nil) | ||
| (wallet.utils/clear-activity-and-scanned-address))] |
There was a problem hiding this comment.
I'm not sure, but I think we're not using the form-2 style components either (?)
Comment on lines
+55
to
+57
| :on-press (fn [] | ||
| (on-press-confirm-add-address-to-save input-value) | ||
| (clear-input)) |
There was a problem hiding this comment.
This could be use-callback function if we want to avoid extra re-renders for the footer button.
Comment on lines
+72
to
+78
| :on-press #(on-press-confirm-address-to-save | ||
| {:ens? ens? | ||
| :adding-address-purpose adding-address-purpose | ||
| :account-name account-name | ||
| :account-color account-color | ||
| :address address | ||
| :theme theme})}} |
There was a problem hiding this comment.
This could also be a use-callback if want to avoid extra re-renders for the bottom-action button.
Comment on lines
+24
to
+40
| on-change-text (fn [new-text] | ||
| (set-validation-message (validate new-text)) | ||
| (set-input-value new-text) | ||
| (reagent/flush) | ||
| (if (and (not (string/blank? new-text)) (nil? (validate new-text))) | ||
| (debounce/debounce-and-dispatch [:wallet/get-address-details new-text] | ||
| 500) | ||
| (rf/dispatch [:wallet/clear-address-activity])) | ||
| (when (and scanned-address (not= scanned-address new-text)) | ||
| (wallet.utils/clear-activity-and-scanned-address))) | ||
| paste-on-input #(clipboard/get-string | ||
| (fn [clipboard-text] | ||
| (on-change-text clipboard-text)))] | ||
| (rn/use-effect (fn [] | ||
| (when-not (string/blank? scanned-address) | ||
| (on-change-text scanned-address))) | ||
| [scanned-address]) |
There was a problem hiding this comment.
Can you explain a bit more why we reagent/flush?
Also it seems that this logic could be refactored into re-frame effects or even co-effects. Thoughts?
Comment on lines
+25
to
+28
| (defn on-press-close | ||
| [] | ||
| (clear-activity-and-scanned-address) | ||
| (rf/dispatch [:navigate-back])) |
There was a problem hiding this comment.
Maybe this could be renamed to something more specific, for example: close-wallet or on-close-wallet?
Comment on lines
+814
to
+818
| (let [result (rf/sub [sub-name])] | ||
| (is (match? (:address result) (:address sample-added-address))) | ||
| (is (match? (:title result) (:title sample-added-address))) | ||
| (is (match? (:description result) (:description sample-added-address))) | ||
| (is (match? (:adding-address-purpose result) (:adding-address-purpose sample-added-address)))))) |
There was a problem hiding this comment.
You might be able to use select-keys and then use match? on two maps?
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 #19796, #19765, #19766, #19764
Summary
This PR refactors the add address to watch component and makes it reusable, It abstracts the definition of the title, subtitle and the event that's going to be executed after the user proceeds after typing/scanning an ETH address or ENS name.
Review notes
The previous add address to watch should be tested to make sure no regressions happened and the addition of saved addresses will be under a feature flag as it's still WIP (The addition is added but the rendering of the accounts isn't here so you can add addresses but they won't show up lolz).
Add address to watch flow:
Add saved address flow:
Testing notes
Please only test that adding addresses to watch isn't broken.
Platforms
Areas that maybe impacted
Adding addresses to watch
Functional
Steps to test
Before and after screenshots comparison
Not required
status: ready