feat: implement summary tag component #17858
Conversation
briansztamfater
force-pushed
the
feat/summary-tag
branch
from
5e3eb47 to
5c539dc
Compare
Jenkins BuildsClick to see older builds (16)
|
briansztamfater
force-pushed
the
feat/summary-tag
branch
from
5c539dc to
70462ad
Compare
briansztamfater
requested review from
vkjr,
smohamedjavid,
J-Son89 and
mmilad75
briansztamfater
changed the title
| :address address | ||
| "") | ||
| customization-color (case type | ||
| :account (:customization-color account-props) |
There was a problem hiding this comment.
imo we should pass customization-color as it's own prop instead 👍
i.e
(defn- view-internal
[{:keys [customization-color ...
address type
theme]
```
| address type | ||
| theme] | ||
| :as props}] | ||
| (let [text (case type |
There was a problem hiding this comment.
I think we're making the api of this component really confusing here. Why not just have a "text" param and handle this outside in application code. although I would probably change "text" to something like "label"
There was a problem hiding this comment.
I agree too, I think this component should only receive the params it needs to work. Right now it's hard to know what keys are expected in those maps (account-props, user-props, network-props, etc) to make this work.
Also a docstring specifying the props supported for each type could work 👍
| {:source (resources/get-token (keyword (string/lower-case (:symbol token-props)))) | ||
| :style {:width 24 | ||
| :height 24}}] | ||
| nil)) |
There was a problem hiding this comment.
I personally would prefer this as something like:
(defn- left-view
[{:keys [type emoji customization-color label image-source
]}]
(case type
:account
[account-avatar/view
{:customization-color customization-color
:size 24
:emoji emoji
:type :default}]
:network
[rn/image
{:source image-source
:style {:width 24
:height 24}}]
:saved-address
[wallet-user-avatar/wallet-user-avatar
{:full-name label
:size :size-24
:customization-color customization-color }]
:collectible
[rn/image
{:source image-source
:style style/collectible-image}]
:user
[user-avatar/user-avatar
{:full-name label
:size :xs
:profile-picture image-source
:customization-color customization-color}]
:token
[rn/image
{:source image-source
:style {:width 24
:height 24}}]
nil))
and also the token image resolving I prefer to do in the application code, as perhaps in some case they are urls and others based on images that we already have in the codebase.
cc @ulisesmac
There was a problem hiding this comment.
Yep, I agree. The API looks cleaner 👍
| :style {:width 24 | ||
| :height 24}}] |
There was a problem hiding this comment.
This could be in the style ns :)
| {:source (resources/get-token (keyword (string/lower-case (:symbol token-props)))) | ||
| :style {:width 24 | ||
| :height 24}}] | ||
| nil)) |
There was a problem hiding this comment.
Yep, I agree. The API looks cleaner 👍
| address type | ||
| theme] | ||
| :as props}] | ||
| (let [text (case type |
There was a problem hiding this comment.
I agree too, I think this component should only receive the params it needs to work. Right now it's hard to know what keys are expected in those maps (account-props, user-props, network-props, etc) to make this work.
Also a docstring specifying the props supported for each type could work 👍
|
@ulisesmac @J-Son89 thanks for the feedback, will refactor and simplify the component API :) |
| [quo.components.tags.summary-tag.view :as summary-tag] | ||
| [test-helpers.component :as h])) | ||
|
|
||
| (h/describe "Summary tag component tests" |
There was a problem hiding this comment.
I'd suggest adding one more test where the component instantiated with empty props, to make sure it doesn't crash
There was a problem hiding this comment.
Similarly, since we have a set of props packed under :token-props, :account-props, their empty state also worth testing.
❗ Right now component crashes the app in following scenarios:
[quo/summary-tag
{:type :token
:token-props {}}]this also:
[quo/summary-tag
{:type :account
:account-props {}}]Didn't test the rest
There was a problem hiding this comment.
@briansztamfater, thanks for the PR!
Could you please also add tests to cover the cases when empty :token-props, :account-props, etc... passed to the component and make sure the component doesn't crash?
Thanks!
briansztamfater
force-pushed
the
feat/summary-tag
branch
from
181849c to
82d2709
Compare
@vkjr Thanks for the feedback, I simplified the API so there is no token-props, account-props, etc anymore. Also refactored tests and added a case where we pass no props so we make sure it doesn't crash. |
|
@Francesca-G can you please review this component? Thanks in advance! |
briansztamfater
force-pushed
the
feat/summary-tag
branch
from
544c1a7 to
1698c39
Compare
There was a problem hiding this comment.
Looks good :)
Here's the review with some minor issues to check
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
feat/summary-tag
branch
from
1698c39 to
5573dad
Compare
|
@Francesca-G thanks for the feedback, updated the text in preview for the token variant (it is just a bug in the preview) and checked the height that is actually 32 points
Proceeding to merge, thanks! |
Signed-off-by: Brian Sztamfater <brian@status.im>
fixes #17848
Summary
Implements Summary tag component
summary.tag.ios.mp4
Platforms
Steps to test
status: ready