Implement "Account origin" Component #17147
Conversation
Jenkins BuildsClick to see older builds (40)
|
| :derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2" | ||
| :user-name "Alisher Yakupov" | ||
| :on-press (h/mock-fn)}]) | ||
| (h/is-truthy (h/get-by-text (i18n/label "origin")))) |
There was a problem hiding this comment.
Please use h/get-by-translation-text instead of i18n/label
| :derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2" | ||
| :user-name "Alisher Yakupov" | ||
| :on-press (h/mock-fn)}]) | ||
| (h/is-truthy (h/get-by-text (i18n/label "on-device")))) |
There was a problem hiding this comment.
h/get-by-translation-text
| :derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2" | ||
| :user-name "Alisher Yakupov" | ||
| :on-press (h/mock-fn)}]) | ||
| (h/is-truthy (h/get-by-text (i18n/label "on-keycard")))) |
There was a problem hiding this comment.
h/get-by-translation-text
| [text/text | ||
| {:weight :medium | ||
| :size :paragraph-1} | ||
| (case type |
There was a problem hiding this comment.
nitpick: I'd rename type since it shadows a Clojure core function.
There was a problem hiding this comment.
I recently opened a PR #17138 to unshadow all core vars & non-core vars, but kept cljs.core/type and cljs.core/name as exceptions in the linter configuration.
I think unshadowing type is difficult in status-mobile, because it's used hundreds of times and it's hard to find a replacement word. I can think of kind, but it's meh IMO. The word type is so ubiquitous in the data structures that we use that I'm not sure it's worth the trouble.
Other words have nice replacements, for example, in the PRs I opened to unshadow:
hash->tx-hash(usually related to the transaction hash)range(as a time period) becomesperiodmeta->metadatacount->amountsubs->subscriptions- And so on
| @@ -323,6 +324,7 @@ | |||
|
|
|||
| ;;;; Wallet | |||
| (def account-card quo2.components.wallet.account-card.view/view) | |||
| (def account-origin quo2.components.wallet.account-origin.view/view) | |||
There was a problem hiding this comment.
This should swap lines with account-overview to be alpha sorted.
There was a problem hiding this comment.
But or should be before ov
| [text/text | ||
| {:weight :medium | ||
| :size :paragraph-1} | ||
| (case type |
There was a problem hiding this comment.
I recently opened a PR #17138 to unshadow all core vars & non-core vars, but kept cljs.core/type and cljs.core/name as exceptions in the linter configuration.
I think unshadowing type is difficult in status-mobile, because it's used hundreds of times and it's hard to find a replacement word. I can think of kind, but it's meh IMO. The word type is so ubiquitous in the data structures that we use that I'm not sure it's worth the trouble.
Other words have nice replacements, for example, in the PRs I opened to unshadow:
hash->tx-hash(usually related to the transaction hash)range(as a time period) becomesperiodmeta->metadatacount->amountsubs->subscriptions- And so on
| [theme] | ||
| {:border-radius 16 | ||
| :border-width 1 | ||
| :border-color (colors/theme-colors colors/neutral-10 |
There was a problem hiding this comment.
The line breaks in theme-colors are unnecessary. Though I understand it's an stylistic/personal choice.
(defn container
[theme]
{:border-radius 16
:border-width 1
:border-color (colors/theme-colors colors/neutral-10 colors/neutral-80 theme)})| :theme theme | ||
| :secondary-color secondary-color}])) | ||
|
|
||
| (defn- card-view |
There was a problem hiding this comment.
In general it's fine to name components without the suffix view (or whatever other suffix). I use this convention myself when I have a binding with a name that conflicts (shadows) the component name, but that's not the case here. Just card seems to be enough because I don't see any binding or var named card.
There was a problem hiding this comment.
You're right, but the reason I used card-view was to match other names in the file. There word list is a reserved-word in clojure, so I had to change it to list-view, therefore, I changed card to card-view
There was a problem hiding this comment.
I would do the same and name it list-view. For the card component I see you wanted to have a consistent naming scheme. Fair enough :)
| :margin-horizontal 20}} | ||
| [quo/account-origin @state]]]))) | ||
|
|
||
| (defn preview |
There was a problem hiding this comment.
Please, update this preview namespace to use the new API that helps us keep these screens DRY(ish). You can find a few dozen examples in the codebase already. For example: status-im2.contexts.quo-preview.dropdowns.dropdown
There was a problem hiding this comment.
@mmilad75, the namespace is not yet using the new code. If you take a look at status-im2.contexts.quo-preview.dropdowns.dropdown (or many others, like status-im2.contexts.quo-preview.list-items.dapp) you will see there's less boilerplate.
We can use the component preview/preview-container. Here's how it should probably be:
(ns status-im2.contexts.quo-preview.wallet.account-origin
(:require [quo2.core :as quo]
[reagent.core :as reagent]
[status-im2.common.resources :as resources]
[status-im2.contexts.quo-preview.preview :as preview]))
(def descriptor
[{:type :select
:key :type
:options [{:key :default-keypair}
{:key :recovery-phrase}
{:key :private-key}]}
{:type :select
:key :stored
:options [{:key :on-device}
{:key :on-keycard}]}])
(defn view
[]
(let [state (reagent/atom {:type :default-keypair
:stored :on-keycard
:profile-picture (resources/get-mock-image :user-picture-male5)
:derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2"
:user-name "Alisher Yakupov"
:on-press #(js/alert "pressed")})]
(fn []
[preview/preview-container {:state state :descriptor descriptor}
[quo/account-origin @state]])))| :type :select | ||
| :key :type | ||
| :options [{:key :default-keypair | ||
| :value "Default Keypair"} |
There was a problem hiding this comment.
You don't need to explicitly pass :value if you think the humanized text deduced from :default-keypair is good enough. In this case it is, :default-keypair will be automatically transformed to "Default keypair". You can also remove redundant :label values.
For reference: here's the function that does this work for us
This is how the descriptor should look like:
(def descriptor
[{:type :select
:key :type
:options [{:key :default-keypair}
{:key :recovery-phrase}
{:key :private-key}]}
{:type :select
:key :stored
:options [{:key :on-device}
{:key :on-keycard}]}])There was a problem hiding this comment.
Very smaller and cleaner. Done
| (defn- list-view | ||
| [{:keys [type stored profile-picture user-name theme secondary-color]}] | ||
| (let [stored-name (if (= :on-device stored) | ||
| (i18n/label "on-device") |
There was a problem hiding this comment.
I know I'm just repeating something you already know :), but translation keys should be keywords and qualified with :t
There was a problem hiding this comment.
My bad, thanks for reminding it. Done
| [utils.i18n :as i18n])) | ||
|
|
||
| (h/describe | ||
| "account origin tests" |
There was a problem hiding this comment.
[Code style] The describe text arg almost always go on the same line as describe.
| (defn- list-view | ||
| [{:keys [type stored profile-picture user-name theme secondary-color]}] | ||
| (let [stored-name (if (= :on-device stored) | ||
| (i18n/label "on-device") |
There was a problem hiding this comment.
Using the keyword with i18n/label is the default approach in a codebase, so we can stick to it. Like (i18n/label :t/on-device), (i18n/label :t/on-keycard)
| @@ -0,0 +1,53 @@ | |||
| (ns status-im2.contexts.quo-preview.wallet.account-origin | |||
There was a problem hiding this comment.
Quo preview can be simplified by using improvements added in this PR. That will make the file shorter and less boilerplate :)
There was a problem hiding this comment.
We can go further @mmilad75. See comment #17147 (comment)
|
Wow, I shouldn't have made a break during the PR review) All my comments became a duplicate 😁 |
| :stored :on-keycard | ||
| :derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2" | ||
| :user-name "Alisher Yakupov" | ||
| :on-press (h/mock-fn)}]) |
There was a problem hiding this comment.
I think passing a mock function to on-press is unnecessary in the tests, unless we are explicitly asserting something about it.
There was a problem hiding this comment.
yep, tests should use the minimum amount of data needed for the test to work imo
| :secondary-color secondary-color}]) | ||
|
|
||
| (def view-internal | ||
| (fn [{:keys [type theme derivation-path on-press] :as props}] |
There was a problem hiding this comment.
This anonymous function is unnecessary, or in other words, view-internal doesn't need to be a form-2 component.
| :margin-horizontal 20}} | ||
| [quo/account-origin @state]]]))) | ||
|
|
||
| (defn preview |
There was a problem hiding this comment.
@mmilad75, the namespace is not yet using the new code. If you take a look at status-im2.contexts.quo-preview.dropdowns.dropdown (or many others, like status-im2.contexts.quo-preview.list-items.dapp) you will see there's less boilerplate.
We can use the component preview/preview-container. Here's how it should probably be:
(ns status-im2.contexts.quo-preview.wallet.account-origin
(:require [quo2.core :as quo]
[reagent.core :as reagent]
[status-im2.common.resources :as resources]
[status-im2.contexts.quo-preview.preview :as preview]))
(def descriptor
[{:type :select
:key :type
:options [{:key :default-keypair}
{:key :recovery-phrase}
{:key :private-key}]}
{:type :select
:key :stored
:options [{:key :on-device}
{:key :on-keycard}]}])
(defn view
[]
(let [state (reagent/atom {:type :default-keypair
:stored :on-keycard
:profile-picture (resources/get-mock-image :user-picture-male5)
:derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2"
:user-name "Alisher Yakupov"
:on-press #(js/alert "pressed")})]
(fn []
[preview/preview-container {:state state :descriptor descriptor}
[quo/account-origin @state]])))
| [card-view theme derivation-path secondary-color on-press])]))) | ||
|
|
||
| (def view | ||
| "Create an accont-origin UI component. |
There was a problem hiding this comment.
btw I like this format for the docstrings much easier to read 👍
| @@ -8,71 +8,63 @@ | |||
| {:type :recovery-phrase | |||
| :stored :on-keycard | |||
| :derivation-path "m / 44’ / 60’ / 0’ / 0’ / 2" | |||
| :user-name "Alisher Yakupov" | |||
| :on-press (h/mock-fn)}]) | |||
| :user-name "Alisher Yakupov"}]) | |||
There was a problem hiding this comment.
nit: maybe let's use a test name, e.g "Test Name"
mmilad75
force-pushed
the
milad/16951
branch
from
24df68c
to
265c537
Compare
Implement "Account origin" Component
|
@mmilad75 Based on https://github.com/status-im/status-mobile/blob/develop/doc/pipeline_process.md > Design review, all follow-up issues have to be created when PR is merged and linked to the original PR. |
No, I didn't. I wasn't aware of this concept. Thanks for letting me know |
fixes #16951
Screen.Recording.2023-08-30.at.15.22.12.mov
Screen.Recording.2023-08-30.at.14.57.00.mov
Platforms