feat: add token tag component (#13599) #13644
Conversation
|
Hey @J-Son89, and thank you so much for making your first pull request in status-react! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Jenkins BuildsClick to see older builds (49)
|
src/quo2/components/token_tag.cljs
Outdated
| :border-radius 100 | ||
| :border-color (if (= (theme/get-theme) :dark) colors/black colors/white) | ||
| :border-width 1 | ||
| :right -20 |
There was a problem hiding this comment.
perhaps there's a better way to get the purchase and checkmark icon sitting in the right place as per the designs 🤔
|
Hi @J-Son89, thank you for the contribution. |
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
7892c51 to
7d8d880
Compare
|
@J-Son89 linting failed, you can run |
|
We should use same naming convention for all token icons. Maybe same as figma names, like |
src/quo2/components/token_tag.cljs
Outdated
| (def themes {:light {:background-color colors/neutral-20} | ||
| :dark {:background-color colors/neutral-80}}) | ||
|
|
||
| (defn get-value-from-size [size, big-option, small-option] |
There was a problem hiding this comment.
No need of commas between parameters
src/quo2/components/token_tag.cljs
Outdated
| [status-im.ui.components.icons.icons :as icons] | ||
| [quo2.components.text :as text])) | ||
|
|
||
| (def themes {:light {:background-color colors/neutral-20} |
There was a problem hiding this comment.
Indentation is a bit off, we either leave a single space, or align values vertically
There was a problem hiding this comment.
cool, I set it to a single space
src/quo2/components/token_tag.cljs
Outdated
| }" | ||
| [_ _] | ||
| (fn [{:keys [token value size icon-name border-color is-required is-purchasable] :or {size :small}}] | ||
| [rn/view {:style (merge (style-container size border-color is-required) (get-in themes [(theme/get-theme)]))} |
There was a problem hiding this comment.
No need to use (get-in themes [(theme/get-theme)]), if not a nested map, you can use get
(get themes (theme/get-theme))
or (fun facts)
(themes (theme/get-theme))
((theme/get-theme) themes))
as both keywords and maps implement IFn (https://clojure.github.io/clojure/javadoc/clojure/lang/IFn.html ) and implement get.
(get themes (theme/get-theme)) is the most readable :)
There was a problem hiding this comment.
awesome, thanks Andrea! :)
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
7d8d880 to
da40c3a
Compare
src/quo2/screens/main.cljs
Outdated
| @@ -23,6 +24,9 @@ | |||
| {:name :quo2-tabs | |||
| :insets {:top false} | |||
| :component tabs/preview-tabs} | |||
| {:name :quo2-token_tag | |||
There was a problem hiding this comment.
better to use quo2-token-tag, so that all screen names looks similar
|
Also result looks different when is required or is Purchasable is enabled, |
I discussed with Pedro from the design team, and the border color for the required icon should be able to be set externally. e.g so that certain groups can use it themed for their group. Although, now I think about it. The purple used in the design should probably be the default color unless another border color is set. |
Yes, I think they always come in purple (two different shades, one for light & one for dark), so no need to make the color customizable, as well because the icons at the moment they come already with that color: https://www.figma.com/file/qLLuMLfpGxK9OfpIavwsmK/Mobile-Iconset?node-id=971%3A71 This one should be the right size, |
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
da40c3a to
27bc509
Compare
I removed border-color and color from the required and purchasable-icons so that these just come from the icons directly. also I added a default value for the required icon (the new icon above) and set the default value of the border-color of the whole tag to match too. |
|
regarding icons naming https://discord.com/channels/624205794384281629/626744384565674034/991236399737098240 |
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
13be046 to
fc620d1
Compare
updated the files I used to match this |
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
49a6ff6 to
da24629
Compare
src/quo2/foundations/colors.cljs
Outdated
| @@ -260,3 +260,5 @@ | |||
|
|
|||
| (defn theme-colors [light dark] | |||
| (if (theme/dark?) dark light)) | |||
|
|
|||
| ;;;;Customisation | |||
There was a problem hiding this comment.
whoops, must remove that too! thanks
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
ecad3c4 to
f114fb5
Compare
src/quo2/components/tag.cljs
Outdated
| (if (= size :big) big-option small-option)) | ||
|
|
||
| (defn tag-container [size border-color] | ||
| (merge {:height (if border-color (get-value-from-size size 34 26) (get-value-from-size size 32 24)) |
There was a problem hiding this comment.
If components are created from the bottom up, like small/base components first, and then used to create new components with the same padding/margin as Figma, then the height will automatically be 32. And if height is not hard-coded then border-width automatically will be applied outside and we don't need to calculate new height as for border-width (as same as you don't need to calculate new width). (no strong opinion, just thinking out loud)
There was a problem hiding this comment.
yeah so I just added another view around this so I could hardcode the height and with of the child view. Which means the border height will be added outside automatically.
src/quo2/components/tag.cljs
Outdated
| :align-items :center | ||
| :flex-direction :row | ||
| :left 0 | ||
| :border-radius 100 |
There was a problem hiding this comment.
hmm, I think I just maxed it out. I guess at some point it makes no odds so I will adjust it down to the right setting
src/quo2/components/tag.cljs
Outdated
| (defn tag "[tag opts \"label\"] | ||
| opts | ||
| { | ||
| :size :small/:big | ||
| :token-img-src :token-img-src | ||
| :token-img-style {} | ||
| :border-color :color | ||
| :text-value string | ||
| :overlay child-elements | ||
| }" | ||
| [_ _] |
There was a problem hiding this comment.
Thanks for adding comments.
From comment it looks like you meant to pass label as argument, but then you used text-value ?
src/quo2/components/token_tag.cljs
Outdated
| (when (or is-required is-purchasable) [rn/view {:style (merge icon-container-styles {:width 14 | ||
| :height 14 | ||
| :background-color border-color | ||
| :border-color (if (= (theme/get-theme) :dark) colors/black colors/white) | ||
| :border-width 1 | ||
| :right (get-value-from-size size -3 -5) | ||
| :bottom (get-value-from-size size (- 32 7 (if is-required 5 4)) (- 24 7 (if is-required 4 2))) ; (- height (icon-height/2) spacing) | ||
| })} | ||
| [icons/icon (if is-required :main-icons2/required-checkmark12 :main-icons2/purchasable12) (merge {:no-color true | ||
| :width (if is-required 7 12) | ||
| :height (if is-required 6 12)} | ||
| (when is-required {:container-style {:margin-right 1}}))]])}])) |
There was a problem hiding this comment.
better to move [rn/view ... to new line. (When it is possible, please, keep lines' length shorter/equal 80 chars.)
There was a problem hiding this comment.
noted, thanks Parvesh :)
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
f114fb5 to
3653b80
Compare
|
@jo-mut i see a tag component in you community PR , could please take a look at this PR |
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
3653b80 to
177d181
Compare
I'll be following onto John's work on communities in some other work so I can also look into making them both referencing the same base tag component |
src/quo2/components/token_tag.cljs
Outdated
| :border-width 1 | ||
| :right (get-value-from-size size -3.75 -5.75) | ||
| :bottom (get-value-from-size size (- 32 7.75 4) (- 24 7.75 2)) ; (- height (icon-height/2) spacing) | ||
| })} |
|
I think all tags are sharing some properties. I implemented a base-tag but I think some properties are also missing on the base-tag. I will look into it |
cool, we can follow up to refactor both of these once this work is merged in. |
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
177d181 to
56e80b8
Compare
|
yeah its just refactoring. As it is looks okay |
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
6af786f to
e774b2b
Compare
src/quo2/components/tag.cljs
Outdated
| :flex-direction :row | ||
| :border-radius 20}) | ||
|
|
||
| (defn tag "[tag opts] |
There was a problem hiding this comment.
thanks Parvesh, I'll adjust 👍
| {:height (get-value-from-size size 32 24) | ||
| :align-items :center | ||
| :flex-direction :row | ||
| :border-radius 20}) |
There was a problem hiding this comment.
Just a suggestion, but we usually indent values inside map (more readable). Example
src/quo2/components/token_tag.cljs
Outdated
| (def icon-container-styles {:display :flex | ||
| :align-items :center | ||
| :justify-content :center | ||
| :position :absolute | ||
| :border-radius 20 | ||
| :margin-left 2}) |
There was a problem hiding this comment.
If possible, please indent all maps
88% of end-end tests have passedNot executed tests (1)Failed tests (10)Click to expandClass TestPublicChatMultipleDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestCommandsMultipleDevicesMerged:
Passed tests (76)Click to expandClass TestSendTxDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestPairingSyncMultipleDevicesMerged:
Class TestOnboardingOneDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestKeycardTxOneDeviceMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestRestoreOneDeviceMerged:
|
J-Son89
force-pushed
the
feat/token-tag-component
branch
2 times, most recently
from
17588f7 to
684e74c
Compare
J-Son89
force-pushed
the
feat/token-tag-component
branch
from
684e74c to
a4b20b3
Compare
fixes #13599
Summary
Implements UI Component - Token tag
This implements and light and dark mode for the token tag component, it does not include the blurred versions of these as discussed with @cammellos and the solution for this needs to be discussed first.
Platforms
Areas that maybe impacted
Test in quo 2 components
Steps to test
status: ready