Refactor add address to watch component and make it generic and use it in add saved addresses screen behind a FF #19797
Conversation
ibrkhalil
changed the title
Jenkins BuildsClick to see older builds (140)
|
ibrkhalil
changed the title
| @@ -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? 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) |
|
|
||
| (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
| (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
| @@ -17,12 +18,26 @@ | |||
| :image (resources/get-themed-image :sweating-man theme) | |||
| :container-style style/empty-container-style}])) | |||
|
|
|||
| (defn on-press | |||
| :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
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
| @@ -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.
| :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}]) | |||
| [{: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.
| ;; 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 | |||
| 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]}] | |||
| [{: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)?
| {: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?
| [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 (?)
| :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.
| :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.
| 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?
| (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?
| (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?
|
Taking over this PR. Marking this PR as a Draft until the reviews are addressed. |
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