Implement account_card and variants #16801
Conversation
Jenkins BuildsClick to see older builds (37)
|
| empty? (= :empty type)] | ||
| [rn/view (style/card customization-color watch-only? metrics? theme) | ||
| [rn/view {:style {:flex-direction :row :align-items :center}} | ||
| [rn/view {:margin-right 8 :margin-top 2 :style (style/loader-view 16 16 watch-only? theme)}] |
There was a problem hiding this comment.
Quick one: missing encapsulating the map inside {:style ...}.
| (let [watch-only? (= :watch-only type) | ||
| empty? (= :empty type)] | ||
| [rn/view (style/card customization-color watch-only? metrics? theme) | ||
| [rn/view {:style {:flex-direction :row :align-items :center}} |
There was a problem hiding this comment.
It would be beneficial if you could move more styles from the view namespace to the style one.
| [rn/view {:style style/watch-only-container} | ||
| [rn/view {:style (style/loader-view 57 8 watch-only? theme)}] | ||
| (when watch-only? [icon/icon :reveal {:color colors/neutral-50 :size 12}])]] | ||
| [rn/view {:margin-top 13 :style (style/loader-view (if empty? 56 80) 16 watch-only? theme)}] |
There was a problem hiding this comment.
This line is a bit difficult to read. Unfortunately the formatter moves the number 13 to the next line bleh, nothing we can do. Suggestion, assoc the value to the style so that we don't leave style props like margin-top outside of the :style map.
[rn/view
{:style (assoc (style/loader-view (if empty? 56 80)
16
watch-only?
theme)
:margin-top
13)}]There was a problem hiding this comment.
I applied in other cases too. Tks!
| account-amount (if (= :empty state) "€0.00" amount) | ||
| account-name (if (= :empty state) "Account 1" name) | ||
| account-percentage (if (= :empty state) "€0.00" percentage-value) | ||
| ] |
There was a problem hiding this comment.
Dangling square bracket. Reminder that dangling parentheses won't be fixed by the formatter.
| theme]}] | ||
| (let [watch-only? (= :watch-only type) | ||
| empty? (= :empty type) | ||
| account-amount (if (= :empty state) "€0.00" amount) |
There was a problem hiding this comment.
I think all these values should be translated @FFFra. Euros don't make sense for most users in the world and Account 1 is also in English.
Numbers should be formatted according to their localization. Take a look at the utils.i18n/format-currency function, it might be just what you need.
There was a problem hiding this comment.
I agree, but as far as I know, fiat currencies and symbol handlers are not defined in this case. I will double-check with the team if I can change that now. Nice catch!
@J-Son89, what do you think?
There was a problem hiding this comment.
Let's discuss this with the team first :)
There was a problem hiding this comment.
I'm checking the codebase to confirm the best approach and will back to you guys
There was a problem hiding this comment.
Nice, yeah, let's always align with designers if the examples are not in Figma. Thanks @FFFra
| :style (style/metrics watch-only? theme)} | ||
| account-percentage] | ||
| (when (not empty?) | ||
| [:<> [rn/view (style/separator watch-only? theme)] |
There was a problem hiding this comment.
[Code style] We almost always add a line break after :<>.
| [quo2.components.markdown.text :as text] | ||
| [quo2.theme :as theme])) | ||
|
|
||
| (defn loading-view |
There was a problem hiding this comment.
This and the other function in this ns can also be private.
| :align-items :center}} | ||
| [text/text | ||
| {:size :heading-1 | ||
| :weight :semi-bold} "Account card"] |
There was a problem hiding this comment.
Missing line break after the closing bracket. https://guide.clojure.style/#vertically-align-fn-args
| [rn/view {:margin-top 13 :style (style/loader-view (if empty? 56 80) 16 watch-only? theme)}] | ||
| (when metrics? | ||
| [rn/view | ||
| {:margin-top 10 |
There was a problem hiding this comment.
Same as the comment I made above, :margin-top should be assoc'ed into the map returned by style/loader-view because style properties should be placed inside the style map. It's not wrong you know, but it's the guideline in status-mobile.
There was a problem hiding this comment.
Here's the Figma frame with the design review.
Please refer to "Review 2", thank you :)
There was a problem hiding this comment.
@FFFra, great work and thanks for not rushing to merge the PR. Take good care of your first PR! I sometimes come back to my first one in status-mobile to remember the "old" days ;)
I'm leaving my approve, although as we discussed I think the component is not 100% ready to shine without proper localization of the money amounts and currencies. I would consider creating a separate issue and merging this PR if you're blocked by external factors (new designs). Since this component has no usages yet, the really important thing IMO is to make sure there's follow-up work on it to make it 100% :)
FFFra
force-pushed
the
ff/account-card-component
branch
from
2ee5801
to
5569564
Compare
FFFra
force-pushed
the
ff/account-card-component
branch
2 times, most recently
from
019613a
to
ebf7cdf
Compare
FFFra
force-pushed
the
ff/account-card-component
branch
4 times, most recently
from
3a0cb5b
to
a036c51
Compare
FFFra
force-pushed
the
ff/account-card-component
branch
from
a036c51
to
874c19a
Compare
fixes #16028
Summary
This PR enhances the wallet accounts functionality, with all variants (Default, Add-account, Watch-only, and Empty) with a 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.
Tests
All tests should run correctly
Platforms
Functional
Steps to test
Please navigate to "Profile and Quo.2.0 Preview" and locate the component under "Wallet/account-card."
Demo light mode
1-Light-Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-07-28.at.18.20.18.mp4
Demo dark mode
2-Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-07-28.at.18.24.06.mp4