FFFra/account card component #16309
FFFra/account card component #16309
Conversation
|
Hey @FFFra, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
| :style {:color (if (and watch-only? (not (colors/dark?))) | ||
| colors/neutral-80-opa-60 | ||
| colors/white-opa-70) | ||
| }}) |
| [text/text (style/account-name watch-only) | ||
| name] | ||
| (if watch-only [icon/icon :reveal {:color colors/neutral-50 :size 12}])] | ||
| ] |
| [rn/view style/watch-only-container | ||
| [text/text (style/account-name watch-only) | ||
| name] | ||
| (if watch-only [icon/icon :reveal {:color colors/neutral-50 :size 12}])] |
There was a problem hiding this comment.
if you use an if without a second condition, then you can use when
i.e
(if condition
(do-something))
should be rewritten as
(when condition
(do-something))
we use if only if you have two branches:
(if condition
(do-something-1)
(do-something-2))
| [text/text (style/metrics watch-only) percentage-value] | ||
| [rn/view (style/separator watch-only)] | ||
| [text/text (style/metrics watch-only) amount]]]] | ||
| ) |
| [plus-button/plus-button | ||
| {:on-press handler | ||
| :customization-color :blue | ||
| }] |
src/quo2/core.cljs
Outdated
| quo2.components.settings.reorder-item.view)) | ||
| quo2.components.wallet.account-card.view | ||
| quo2.components.settings.reorder-item.view | ||
| )) |
| :customization-color :army | ||
| :watch-only false | ||
| :type :default | ||
| } |
| :amount "€570.24" | ||
| :watch-only true | ||
| :type :watch-only | ||
| } |
| {:id 3 | ||
| :type :add-account | ||
| :handler #(js/alert "Add account pressed") | ||
| }]) |
| :separator [separator] | ||
| :render-fn quo/account-card | ||
| :showsHorizontalScrollIndicator false | ||
| }]]) |
| [test-helpers.component :as h])) | ||
|
|
||
| (h/describe "Account_card tests" | ||
| (def john-doe-name "John Doe") |
There was a problem hiding this comment.
defs generally go top level, so above and outside (h/describe
Not sure exactly on this case as h/describe is a macro, so just double checking with @ilmotta , but I am fairly certain that you want to have it outside (I'd use let if inside, as looks more customary)
There was a problem hiding this comment.
I agree with @cammellos, the def and defn should move outside h/describe ideally. I understand in Javascript it's customary to define variables and even functions inside each (nested) describe block, but in Clojure it doesn't translate as idiomatic code. I also prefer the vars outside to make REPL usage more friendly, since it's a bit easier to eval the top level s-exp.
The local binding I guess doesn't work well here because the value needs to be used by two tests, and surrounding with a let both h/test makes the code less readable due to nesting (a matter of taste perhaps).
Per @cammellos's suggestion:
(def john-doe-name "John Doe")
(defn get-test-data
[type watch-only]
{:name john-doe-name
:balance "€1,000.00"
:percentage-value "50%"
:amount "€500.00"
:customization-color :blue
:watch-only watch-only
:type type})
(h/describe "Account_card tests"
(h/test "Renders normal"
(let [data (get-test-data :default false)]
(h/render [account-card/view data])
(h/is-truthy (h/get-by-text john-doe-name))))
(h/test "Renders watch-only"
(let [data (get-test-data :watch-only true)]
(h/render [account-card/view data])
(h/is-truthy (h/get-by-text john-doe-name))))
...)Also the code is not indented properly and it was confusing for me to read it. @FFFra you can fix this with make lint-fix.
|
|
||
| (defn user-account-view | ||
| [{:keys [name balance percentage-value amount customization-color watch-only type]}] | ||
| [rn/view |
| [rn/view | ||
| [rn/view (style/card customization-color watch-only) | ||
| [rn/view style/profile-container | ||
| [icon/icon :contact {:size 20}] |
There was a problem hiding this comment.
:size 20 is default, so its redundant here
| [rn/view style/watch-only-container | ||
| [text/text (style/account-name watch-only) | ||
| name] | ||
| (if watch-only [icon/icon :reveal {:color colors/neutral-50 :size 12}])] |
| ] | ||
| [text/text (style/account-value watch-only) balance] | ||
| [rn/view style/metrics-container | ||
| [rn/view {:margin-right 5.5} |
There was a problem hiding this comment.
margin should be integer i guess
There was a problem hiding this comment.
In this case I was just following figma
| {:size :paragraph-2 | ||
| :weight :medium | ||
| :margin-top 4 | ||
| }) |
There was a problem hiding this comment.
We follow the Clojure Style Guide, so we have a final \n https://guide.clojure.style/#terminate-files-with-a-newline. Please, fix all files in the PR without terminating newline characters.
There was a problem hiding this comment.
Tks, to avoid this in the future I added that to my VSCode Settings
There was a problem hiding this comment.
Great, that's the way to go! Thanks
| [text/text {:size :heading-1 :weight :semi-bold :style {:margin-bottom 40}} "Account card"] | ||
| [rn/flat-list | ||
| {:data mock-data | ||
| :keyExtractor #(str (:id %)) |
There was a problem hiding this comment.
We can use kebab style in this map.
| :border-width 1 | ||
| :border-color (if watch-only? | ||
| (colors/theme-colors colors/neutral-80-opa-5 colors/white-opa-5) | ||
| (colors/custom-color :army 50)) |
There was a problem hiding this comment.
should this be :army or customization-color passed as a param?
| [rn/view style/add-account-container | ||
| [plus-button/plus-button | ||
| {:on-press handler | ||
| :customization-color :blue |
There was a problem hiding this comment.
customization-color should be a prop that's passed in 👍
| [text/text style/add-account-title (i18n/label :t/add-account)]]) | ||
|
|
||
| (defn view | ||
| [{:keys [name balance percentage-value amount customization-color handler watch-only add-account |
There was a problem hiding this comment.
watch-only prop is redundant as there is type watch-only, you can create an internal let var if you think it make it easier to read.
(defn user-account-view
[{:keys [name balance percentage-value amount customization-color type]}]
(let [watch-only? (= :watch-only type)]
...
| @@ -0,0 +1,39 @@ | |||
| (ns quo2.components.wallet.account-card.account-card-spec | |||
There was a problem hiding this comment.
namespace should be:
quo2.components.wallet.account-card.component-spec
| (def add-account-container | ||
| {:width 161 | ||
| :height 88 | ||
| :border-color (colors/theme-colors colors/neutral-20 colors/white-opa-5) |
There was a problem hiding this comment.
If the theme changes the map won't be updated because colors/theme-colors relies on a reactive atom.
There was a problem hiding this comment.
that is can you make this a defn. e,g
(defn [] add-account-container
{:width 161
:height 88
...
| :horizontal true | ||
| :separator [separator] | ||
| :render-fn quo/account-card | ||
| :showsHorizontalScrollIndicator false}]]) |
There was a problem hiding this comment.
The keyword should use kebab case.
There was a problem hiding this comment.
This was removed due to new implementation. But you are right =)
| [watch-only?] | ||
| {:style {:color (text-color watch-only?) | ||
| :margin-left 5} | ||
| :size :paragraph-2 |
There was a problem hiding this comment.
These are not style properties. Please move size and weight outside this namespace. Vars metrics, account-value and add-account-title also should be changed.
| [text/text {:size :heading-1 :weight :semi-bold :style {:margin-bottom 40}} "Account card"] | ||
| [rn/flat-list | ||
| {:data mock-data | ||
| :key-extractor #(str (:id %)) |
There was a problem hiding this comment.
just :d will do
:key-extractor :id
since :id is already a function as it implementsiFn https://cljs.github.io/api/cljs.core/IFn
and I think id is already a string
There was a problem hiding this comment.
Tks! I will apply it if we decide to also include the flat list to the variants preview
|
hey @FFFra still working on this ? |
|
@flexsurfer, yep @FFFra is still working here 👍 |
There was a problem hiding this comment.
@FFFra, the PR looks almost good to me, but there's one important thing I think should be done.
Recently, it was decided that we'll try to get inputs/reviews from designers more often in the development process, even before merging some PRs.
For a designer to review your PR build, it's important you improve the Quo Preview screen to support all the different Figma states. This is also useful for devs to review your component. I tried to review this Account Card component in the UI, but it's way too inconvenient when everything is hardcoded and I have to run your branch locally to make code changes.
Good news is you can find many Quo previews examples in the repo :)
| (case type | ||
| :watch-only [user-account-view props] | ||
| :add-account [add-account-view props] | ||
| :default [user-account-view |
There was a problem hiding this comment.
This line can be joined with the next one to look like [user-account-view props]
| [text/text | ||
| {:size :paragraph-2 | ||
| :weight :medium | ||
| :style {:margin-top 4}} (i18n/label :t/add-account)]]) |
There was a problem hiding this comment.
We have some exceptions, but most of the time we try to build Design System components that don't use i18n directly because we want them to be translation agnostic.
In this case, in Figma we see the "Add account" text is in a layer named Title, which is a good indicator that when the component is of type :add-account it could use an argument named title.
Additionally, the second argument to text should be in the next line https://guide.clojure.style/#vertically-align-fn-args
There was a problem hiding this comment.
Just to make things clearer @FFFra, this is more like a nice to have IMO, not so much of a strong requirement. As I said, we have exceptions in the component lib, so it's fine to leave the code as is.
| [reagent.core :as reagent] | ||
| [utils.collection] | ||
| [status-im2.contexts.quo-preview.preview :as preview] | ||
| )) |
There was a problem hiding this comment.
tks, that should be fixed with lint-fix. Im fixing now.
There was a problem hiding this comment.
@FFFra, unfortunately for us, we have configured zprint in such a way as to respect newlines, which means the tool thinks you want the parentheses in a separate line. The tradeoff is that we give devs more room to format their code, but we often have to catch these formatting issues manually in PRs. Unfortunately lint-fix won't help.
| [icon/icon :i/check {:color colors/white :size 16}]]] | ||
| [rn/view {:style {:flex 1}} | ||
| [preview/customizer state descriptor]] | ||
| (if (:show-flatlist @state) |
There was a problem hiding this comment.
We should strictly follow the Design System. The Account Card component is a single component with many variations. The fact that it can be displayed in a horizontally scrollable list is not specified in the Figma component, this is actually a feature of the real wallet screen using the component.
This is also important to not confuse designers reviewing the preview area.
Therefore, I think we should remove the flatlist option.
There was a problem hiding this comment.
I understand your point, but that was required before. I will talk to the team and come up with a decision
There was a problem hiding this comment.
There was a problem hiding this comment.
It's removed. I also updated the videos for this PR =)
There was a problem hiding this comment.
I see guys, thanks. So next time it would help if we have the requirements up-to-date in the PR description, in case they override the guidelines/conventions.
| :size :paragraph-2 | ||
| :style (style/metrics watch-only?)} amount]]]])) | ||
|
|
||
| (defn add-account-view |
There was a problem hiding this comment.
Please, make all vars private, except view. For defn we use defn- (not the metadata ^:private).
| [utils.i18n :as i18n] | ||
| [quo2.components.markdown.text :as text])) | ||
|
|
||
| (defn user-account-view |
There was a problem hiding this comment.
Usually we don't append view or component suffixes unless there's a naming conflict and the function name would shadow a parameter. So just user-account is enough.
| :weight :medium | ||
| :style {:margin-top 4}} (i18n/label :t/add-account)]]) | ||
|
|
||
| (defn view |
There was a problem hiding this comment.
We are gradually making DS components theme aware by using quo2.theme/with-theme. You can find some examples already, e.g. quo2.components.dividers.divider-label.
The way it works is that the HoF passes the current theme set by a React Context, and then in the component you use this theme parameter whenever you need to use a color or anything that depends on the current theme. In practice, almost all calls to colors/theme-colors should pass a third argument theme (the current theme).
So there are some changes you need to make in this file and in the style.cljs one.
|
Most of the changes are done, some questions I will clarify asap |
There was a problem hiding this comment.
@FFFra, I made just two minor comments that need a fix, but I'm approving now, since the code LGTM.
Before proceeding with the merge, it's important to get a designer to review & approve your PR, particularly because we're introducing a new component in the code. @Francesca-G is the right person for that :)
|
|
||
| (defn metrics | ||
| [watch-only?] | ||
| {:color (if (and watch-only? (not (colors/dark?))) |
There was a problem hiding this comment.
This also needs to be fixed just like you did in text-color.
| [] | ||
| {:width 161 | ||
| :height 88 | ||
| :border-color (colors/theme-colors colors/neutral-20 colors/white-opa-5) |
There was a problem hiding this comment.
This call to colors/theme-colors also need to be fixed to accept a theme argument.
Jenkins BuildsClick to see older builds (8)
|
|
@Francesca-G, @FFFra, the PR builds are available now for design review. @cammellos added @FFFra as a contributor in read-only mode, so the builds could be generated. @FFFra, the lint check failed in the CI, you can fix with |
There was a problem hiding this comment.
Here's the Figma frame with the design review comments @ilmotta @FFFra :)
Thank you, Francesca. I'm afraid I cannot access the file, could you please grant me access? Cheers! |
|
cc @Francesca-G |
Hi @FFFra, sorry for the late response. I spoke to Pedro and he gave your email access to our Figma, so you should be able to view the file now, if not please do let me know :) |
Thank you! I'm taking and already understood the problem. Gonna add you on Discord just in case I have some questions. |
FFFra
force-pushed
the
ff/account-card-component
branch
from
f8f0c42
to
3ade464
Compare
|
hey @FFFra is this still relevant? |
fixes #16028
Summary
This PR enhances the wallet accounts functionality, featuring a new slider, add-account feature with alert notifications, and dynamic preview mode. It supports both light and dark modes, includes comprehensive unit tests, and aligns with the provided design specifications. Please refer to the attached demo for a practical demonstration. Feedback and suggestions are most welcome.
...
Testing notes
All tests are expected to pass successfully
Platforms
Functional
Steps to Test
Please navigate to "Profile and Quo.2.0 Preview" and locate the component under "Wallet/account-card."
Design
The components should look like this:
Edit
I've removed the FlatList as agreed and updated the associated videos accordingly; please review.
##Light Mode
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-07-12.at.17.15.45.mp4
##Dark mode
Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-07-12.at.17.18.04.mp4