Wallet activity component #17141
Wallet activity component #17141
Conversation
Jenkins BuildsClick to see older builds (31)
|
vkjr
force-pushed
the
wallet-activity-component-copy
branch
from
5a01000
to
21da421
Compare
| (def icon-container | ||
| {:width 32 | ||
| :height 32 | ||
|
|
| {:margin-left 8}) | ||
|
|
||
| (def content-line | ||
| {:flex-direction :row :margin-top 2 :align-items :center}) |
There was a problem hiding this comment.
Preferably put each key / value in a new line
| (colors/theme-colors colors/neutral-100 colors/white theme)) | ||
|
|
||
| (def icon-status-container | ||
| {:position :absolute :bottom 0 :right 0}) |
There was a problem hiding this comment.
Same here, put each key / value in a new line
| {:weight :medium | ||
| :size :label | ||
| :style (style/transaction-counter theme)} | ||
| (str "x" counter)]]) |
There was a problem hiding this comment.
IMO is better to use localized strings here, because of future RTL languages support
| :on-press on-press | ||
| :on-press-in (fn [] (reset! pressed? true)) | ||
| :on-press-out (fn [] (reset! pressed? false))} | ||
|
|
| :on-press-in (fn [] (reset! pressed? true)) | ||
| :on-press-out (fn [] (reset! pressed? false))} |
There was a problem hiding this comment.
You can use #() syntax here
There was a problem hiding this comment.
@briansztamfater, some devs prefer to avoid #() when there are no arguments being used. I don't always follow this idea, but I do tend to avoid #() in the cases where there's no function argument because this clearly communicates to the reader that they don't need to "search" for a % symbol somewhere. When I see #() I always search for % just to find out it's not there many times.
Some devs actually hate #() and others love it and request such a syntax in other Lisps. It's funny 🤷🏼
| [rn/view | ||
| {:style {:flex-direction :row}} | ||
| [transaction-icon-view props] | ||
| [rn/view ;content |
| [transaction-icon-view props] | ||
| [rn/view ;content | ||
| {:style style/content-container} | ||
|
|
translations/en.json
Outdated
| @@ -625,7 +625,8 @@ | |||
| "follow-your-interests": "Jump into a public chat and meet new people", | |||
| "follow": "Follow", | |||
| "free": "↓ Free", | |||
| "from": "From", | |||
| "From": "From", | |||
There was a problem hiding this comment.
Definitely not a strong opinion, but IMO is better to maintain one lowercase string and capitalize dynamically.
There was a problem hiding this comment.
I'm not sure you actually want that. I imagine capitalising in different languages can have different/ unexpected effects and I think it's best to leave to the translation string to handle that. Could be wrong on this ofc 🤷♂️
There was a problem hiding this comment.
Given that the translation keys end up being keywords in our codebase :t/<something>, I think we should just consider the idiomatic Clojure code to decide what to do. In Clojure, keywords are case sensitive, but I've never seen any usage of keywords with mixed case, therefore they should be lower case.
There was a problem hiding this comment.
Also didn't like that approach, but we already used it in few times in translations.
But Instead of using capitalized keywords or hoping that (capitalize) won't affect translation, we can use keywords like :t/from-capitalized. I believe they will be obvious enough)
There was a problem hiding this comment.
Good idea @vkjr! The fact that status-mobile has upper cased keywords doesn't come as a surprise because many contributors aren't from a Clojure background (not saying it's bad, just stating a probable reason).
| (fn | ||
| [{:keys [state theme blur? | ||
| on-press | ||
| first second third fourth |
There was a problem hiding this comment.
one thought, is first not a cljs keyword? same with second etc?
There was a problem hiding this comment.
perhaps, first-value etc? 🤔
There was a problem hiding this comment.
Correct @J-Son89, and this will cause a linter failure after I merge #17138. If this PR is merged before the one I opened, I can fix the linter errors, no worries. But it's better if @vkjr can fix this himself in this PR so he can come up with the names he prefers. All cljs.core/** vars will be protected against shadowing from now on.
Well, except all the code inside rf/defn. Be careful 🤷🏼 code inside that macro is not linted at all.
| :type :select | ||
| :options context-tags}])) | ||
|
|
||
| (defn view |
There was a problem hiding this comment.
actually here I think the standard for the preview screens became preview. it's not documented or anything but I noticed that's what it's been shortened to in @ilmotta's pr
There was a problem hiding this comment.
@J-Son89, I went the other way in the recent PRs refactoring preview namespaces. Since our convention for namespaces exposing a single component var is to name them view, I opted to use this idea for preview components too.
| @@ -367,7 +368,9 @@ | |||
| {:name :token-input | |||
| :component token-input/preview} | |||
| {:name :wallet-overview | |||
| :component wallet-overview/preview-wallet-overview}] | |||
| :component wallet-overview/preview-wallet-overview} | |||
| {:name :wallet-activity | |||
There was a problem hiding this comment.
can we keep this alpha sorted, i.e wallet-activity should be before wallet-overview, right? - same with the requires ^ ^
| (:require [quo2.components.wallet.wallet-activity.view :as wallet-activity] | ||
| [test-helpers.component :as h])) | ||
|
|
||
| (def props |
There was a problem hiding this comment.
nit, is this def necessary? - seems easier to use an empty map and avoid the assoc on line 16
| @@ -101,7 +101,7 @@ | |||
| [react/nested-text | |||
| {:style {:color colors/gray} | |||
| :ellipsize-mode :middle | |||
| :number-of-lines 1} (i18n/label :t/to) " " | |||
| :number-of-lines 1} (i18n/label :t/To) " " | |||
There was a problem hiding this comment.
@vkjr, I think we shouldn't use uppercased keywords in status-mobile . If we need to differentiate between keywords that already exist, we can just give them a different name. The translation key doesn't need to match the text. Mixed case keywords are too alien in Clojure, I've never seen this.
| :descriptor descriptor | ||
| :blur? (:blur? @component-state) | ||
| :show-blur-background? true} | ||
| [rn/view {:style {:align-self :center}} |
There was a problem hiding this comment.
FYI: preview-container accepts a component-container-style to avoid the common need to wrap the children in a view. I've seen a handful of previews that could take advantage of this prop.
Maybe component-container-style is too long of a name, I guess just container-style works well. I'll try to remember to rename this if nobody does before me.
vkjr
force-pushed
the
wallet-activity-component-copy
branch
from
21da421
to
cf8c35a
Compare
|
@briansztamfater, @J-Son89, @ilmotta thank you for the review notes! addressed them) |
|
I believe that one can be moved directly to design review, move it once it is ready, thank you @vkjr ! |
|
@Francesca-G, could you please take a look at this one? |
|
blur style doesn't seem to work, when I select it this happens. 
|
|
@Francesca-G, sorry, fixed, should be good now) |
There was a problem hiding this comment.
Nice job, here are some comments about a couple of details :)
|
@Francesca-G, thanks for the review! |
vkjr
force-pushed
the
wallet-activity-component-copy
branch
from
45593c5
to
c10faf2
Compare
vkjr
force-pushed
the
wallet-activity-component-copy
branch
from
0e6a858
to
00c7c54
Compare
* Wallet activity component * Component description added * removed unnecessary piece * lint fix * Review notes * fix issue with blur preview * lint fix
@Francesca-G, could you please check the link you provided? I see there a comments for the transaction summary component, but see no comments for the wallet activity component. Thanks! |
fixes #16609
Summary
Designs
Wallet activity component implemented
Review notes
Designs have different set of properties for this component, but they imply that component contains business logic. From a developer standpoint it is a set of configurable context tag components with an additional info.
Testing notes
status: ready