feat: implement saved address list item component #17398
Conversation
briansztamfater
changed the base branch from
develop
to
feat/account-list-item
briansztamfater
force-pushed
the
feat/account-list-item
branch
from
0f6294b to
c7e3aa4
Compare
Jenkins BuildsClick to see older builds (24)
|
briansztamfater
force-pushed
the
feat/saved-address-item
branch
2 times, most recently
from
2bcbe1a to
611e1f6
Compare
briansztamfater
changed the title
briansztamfater
changed the title
briansztamfater
requested review from
alwx,
vkjr,
FFFra,
tumanov-alex,
smohamedjavid,
J-Son89,
mmilad75 and
mohsen-ghafouri
briansztamfater
force-pushed
the
feat/saved-address-item
branch
from
dac8a82 to
f1e2fb8
Compare
briansztamfater
changed the base branch from
feat/account-list-item
to
develop
briansztamfater
force-pushed
the
feat/saved-address-item
branch
2 times, most recently
from
ac787c0 to
0961568
Compare
| (ns quo2.components.list-items.saved-address.component-spec | ||
| (:require [test-helpers.component :as h] | ||
| [quo2.components.list-items.saved-address.view :as saved-address] | ||
| [quo2.foundations.colors :as colors])) |
There was a problem hiding this comment.
Yeah, actually, I just wanted to test backgroundColor, not the entire style, but I didn't find a better way to do it. Of course I am open to change the approach if you have any suggestion, or to remove these tests entirely if they don't add too much value. Honestly, I don't have a strong opinion about this and I am not 100% sure about these tests.
There was a problem hiding this comment.
Yeah, I remember those discussions @OmarBasem. I tend to almost never say, "don't test that" because usually if the developer went into the trouble of writing a unit test that was useful for them to solve a problem, that probably means it's going to be valuable to other devs in the future.
I don't have a strong opinion, but if a developer wants to test some styles because they do more complicated calculations, that's good IMO. We just don't need to make that a rule, since it's probably an exception.
There was a problem hiding this comment.
Updated to just check for backgroundColor which is the only style prop that needs to be tested 👍
There was a problem hiding this comment.
sure, all sounds good to me 👍
| theme] | ||
| :or {customization-color :blue | ||
| blur? false}}] | ||
| (let [names (string/split (:name user-props) " ") |
There was a problem hiding this comment.
Note that string/split will return [""] when user-props is nil. I haven't checked if this is a problem, but it seems better if an empty seq was returned for your use case, otherwise a check like (seq names) would return true.
(string/split nil " ") ; => ("")Something like this may be better:
(remove string/blank? (string/split nil " ")) ; => ()There was a problem hiding this comment.
Thanks for the suggestion, I added some extra validation because this suggestion made the avatar component to crash if nil is passed as first or last name
| :title "Alisher Yakupov" | ||
| :address "0x21a...49e" | ||
| :on-options-press #(js/alert "Options button pressed!")})] | ||
| (fn [] [preview/preview-container |
There was a problem hiding this comment.
A line break should be added after [].
| :show-blur-background? true | ||
| :blur-dark-only? true} | ||
| [quo/saved-address | ||
| (merge @state |
There was a problem hiding this comment.
assoc is more precise here since you always overwrite :user-props.
There was a problem hiding this comment.
Yup, updated with assoc instead of merge
| :on-press-in on-press-in | ||
| :on-press-out on-press-out | ||
| :accessibility-label :container} | ||
| [rn/view {:style style/left-container} |
There was a problem hiding this comment.
Just a suggestion to break the tree to reduce the re-rendering overhead.
The difference here is that Reagent won't re-compute left-container when state, timer, active?, on-press, and on-options-press are changed.
diff --git a/src/quo2/components/list_items/saved_address/view.cljs b/src/quo2/components/list_items/saved_address/view.cljs
index ace513201..e21d19d02 100644
--- a/src/quo2/components/list_items/saved_address/view.cljs
+++ b/src/quo2/components/list_items/saved_address/view.cljs
@@ -9,6 +9,29 @@
[quo2.components.icon :as icon]
[clojure.string :as string]))
+(defn left-container
+ [{:keys [blur? theme name address customization-color]}]
+ (let [names (string/split name " ")
+ first-name (first names)
+ last-name (last names)]
+ [rn/view {:style style/left-container}
+ [wallet-user-avatar/wallet-user-avatar
+ {:size :medium
+ :f-name first-name
+ :l-name last-name
+ :color customization-color}]
+ [rn/view {:style style/account-container}
+ [text/text
+ {:weight :semi-bold
+ :size :paragraph-1}
+ name]
+ [text/text {:size :paragraph-2}
+ [text/text
+ {:size :paragraph-2
+ :weight :monospace
+ :style (style/account-address blur? theme)}
+ address]]]]))
+
(defn- internal-view
[]
(let [state (reagent/atom :default)
@@ -23,10 +46,7 @@
theme]
:or {customization-color :blue
blur? false}}]
- (let [names (string/split (:name user-props) " ")
- first-name (first names)
- last-name (last names)
- on-press-out (fn []
+ (let [on-press-out (fn []
(let [new-state (if @active? :default :active)]
(when @timer (js/clearTimeout @timer))
(reset! timer nil)
@@ -40,23 +60,12 @@
:on-press-in on-press-in
:on-press-out on-press-out
:accessibility-label :container}
- [rn/view {:style style/left-container}
- [wallet-user-avatar/wallet-user-avatar
- {:size :medium
- :f-name first-name
- :l-name last-name
- :color (:customization-color user-props)}]
- [rn/view {:style style/account-container}
- [text/text
- {:weight :semi-bold
- :size :paragraph-1}
- (:name user-props)]
- [text/text {:size :paragraph-2}
- [text/text
- {:size :paragraph-2
- :weight :monospace
- :style (style/account-address blur? theme)}
- (:address user-props)]]]]
+ [left-container
+ {:blur? blur?
+ :theme theme
+ :name (:name user-props)
+ :address (:address user-props)
+ :customization-color (:customization-color user-props)}]
[rn/pressable
{:accessibility-label :options-button
:on-press #(when on-options-press
There was a problem hiding this comment.
Agree, updated implementation
| (let [state (reagent/atom {:type :default | ||
| :customization-color :blue | ||
| :account-color :pink | ||
| :title "Alisher Yakupov" |
There was a problem hiding this comment.
The descriptor should allow the dev/designer to at least change title as well.
briansztamfater
force-pushed
the
feat/saved-address-item
branch
from
0961568 to
b8128e4
Compare
|
@Francesca-G can you review this component please? |
| (defn- background-color | ||
| [{:keys [state blur? customization-color]}] | ||
| (cond (and (or (= state :pressed) (= state :selected)) (not blur?)) | ||
| (colors/custom-color customization-color 50 5) |
There was a problem hiding this comment.
should these not be themed? 50 for light and 60 for dark ? 🤔
There was a problem hiding this comment.
Based on Figma, it uses the same color for both themes
|
|
||
| (defn- left-container | ||
| [{:keys [blur? theme name address customization-color]}] | ||
| (let [names (remove string/blank? (string/split name " ")) |
There was a problem hiding this comment.
I believe we have some utility function to handle this already?
ah I just checked, we have this for initials but not for names (as far as I can see)
get-initials in src/utils/string.cljs
perhaps we add this method there too? 🤔
cc @ulisesmac
Signed-off-by: Brian Sztamfater <[email protected]>
briansztamfater
force-pushed
the
feat/saved-address-item
branch
from
388ac8d to
981b5e5
Compare
Signed-off-by: Brian Sztamfater <[email protected]>
Signed-off-by: Brian Sztamfater <[email protected]>
fixes #17302
Summary
Implement Saved Address List Item component
Screenshots
Platforms
Steps to test
status: ready