feat: implement saved contact address list item component #17400
Conversation
Jenkins BuildsClick to see older builds (47)
|
briansztamfater
force-pushed
the
feat/saved-contact-address-item
branch
from
f803294
to
a1b3c02
Compare
briansztamfater
changed the title
briansztamfater
requested review from
vkjr,
FFFra,
tumanov-alex,
smohamedjavid,
J-Son89,
mmilad75 and
mohsen-ghafouri
|
|
||
| (defn- f-internal-view | ||
| [] | ||
| (let [state (reagent/atom :default) |
There was a problem hiding this comment.
I think we need to expose the state prop to control the component from the parent. Maybe, just for the active state. WYDT?
There was a problem hiding this comment.
Hmm I think we would not want to expose the whole state atom, but I could add a boolean prop to set if we want to set active state? WDYT?
There was a problem hiding this comment.
Oh, no. Not the atom. I meant the state prop (Figma 1-1).
Most likely we might need a prop for the selected state. Right now, the component handles it but we need to set it from the screen which uses this component.
| {:state @state :customization-color customization-color}) | ||
| :on-press-in on-press-in | ||
| :on-press-out on-press-out | ||
| :accessibility-label :container} |
There was a problem hiding this comment.
I believe we will need an on-press prop.
There was a problem hiding this comment.
I added it, not too much change in the behavior from calling it in on-press-out, but it feels more natural by calling it in on-press 👍
There was a problem hiding this comment.
I agree, Brian, but we need to rely on the on-press-out for the animations.
I thought we would need an on-press for handling any events (probably navigation) on press of the component.
| {:no-color true | ||
| :size 12 | ||
| :color (colors/theme-colors | ||
| (colors/custom-color :blue 50) | ||
| (colors/custom-color :blue 60) | ||
| theme)}]]] |
There was a problem hiding this comment.
The color won't be applied if we pass true to the no-color prop.
P.S. If the icon has two colours, we need to use SVG. (ref quo2.components.icons.svg ns)
There was a problem hiding this comment.
True! Thanks for the suggestion, added the icon as SVG as I need to slightly change the backgrond color while preserving the white shapes inside
| {:size :paragraph-2 | ||
| :weight :monospace | ||
| :style (style/account-address theme)} | ||
| (:address account-props)]] |
There was a problem hiding this comment.
It's good to have the account-props de-structured for better readability and maintainability.
| [rn/view {:accessibility-label :check-icon} | ||
| [icon/icon :i/chevron-right | ||
| {:size 20 | ||
| :color (colors/theme-colors colors/neutral-50 | ||
| colors/neutral-40 | ||
| theme)}]]])))) |
There was a problem hiding this comment.
- I believe we will show the
chevron-righticon if we have more than one account. - If the wrapper view is used only for accessibility, we can remove it and pass the
accessibility-labelto the icon.
There was a problem hiding this comment.
True! Nice catch, fixed it
|
|
||
| (defn- internal-view | ||
| [props] | ||
| [:f> f-internal-view props]) |
There was a problem hiding this comment.
@briansztamfater - any reason why we use a functional component here?
There was a problem hiding this comment.
No, probably some copy-paste leftover, removed it
|
|
||
| (defn- background-color | ||
| [{:keys [state customization-color]}] | ||
| (cond (or (= state :pressed) (= state :selected)) |
There was a problem hiding this comment.
should this not be themed? 🤔
There was a problem hiding this comment.
Hmm its just the customization color with different opacities with not difference on the themes, so I think not
| {:no-color true | ||
| :size 12 | ||
| :color (colors/theme-colors | ||
| (colors/custom-color :blue 50) |
There was a problem hiding this comment.
surely this is also not hardcoded as :blue right? and it needs to be customization-color instead?
There was a problem hiding this comment.
I changed it to use primary colors, it has some minor variation in the icon background, not supposed to be customizable
|
|
||
| (h/describe "List items: saved contact address" | ||
| (h/test "default render" | ||
| (h/render [saved-contact-address/view {:accounts (repeat 1 account)}]) |
There was a problem hiding this comment.
I'd suggest having one test rendering component without any props.
Some components tend to crash on this and that makes dev life harder when you try to use them later without knowing correct props.
There was a problem hiding this comment.
Yeah, this test it supposed to have no props, nice catch! Fixed it
briansztamfater
force-pushed
the
feat/saved-contact-address-item
branch
2 times, most recently
from
2b3f6a7
to
846f0a5
Compare
| :color (colors/theme-colors colors/primary-50 colors/primary-60 theme) | ||
| :color-2 colors/white}]]] | ||
| (if account-props | ||
| [account account-props theme] |
There was a problem hiding this comment.
Maybe it's cleaner to assoc the map here instead of theme as it's own param
There was a problem hiding this comment.
Sure, updated with assoc
|
@Francesca-G please review this component 🙏 |
There was a problem hiding this comment.
Here's the design review
Adding follow up required for minor fixes :)
Thanks for testing @Francesca-G, I've adjusted the icon centering a bit, but the identicon stuff inolves implementing functionality in User Avatar component (and probably the backend) so I will create another issue to track that |
briansztamfater
force-pushed
the
feat/saved-contact-address-item
branch
2 times, most recently
from
e089f4d
to
ebfb6bf
Compare
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/saved-contact-address-item
branch
from
ebfb6bf
to
8d235f6
Compare
|
BTW @Francesca-G I'll merge this one, feel free to review the latest build, I'll create a new PR if you find other issues 👍 |
fixes #17387
Summary
Implement Saved Contact Address List Item component
Screenshots
Platforms
Steps to test
status: ready