-
Notifications
You must be signed in to change notification settings - Fork 986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: quo2 implement amount input component #18687
Changes from 15 commits
6eea6c5
d221e97
1b29ad9
334744f
8fcc03d
0087420
d46daf6
de3ff53
73bda8e
797e696
17e594a
80e0070
409fef2
8430a37
044a717
d6acecc
cb3d246
84dca31
23f805a
c9d8e4a
9eb522e
64683ab
ecd3a89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
(ns quo.components.wallet.amount-input.component-spec | ||
(:require | ||
[oops.core :as oops] | ||
[quo.components.wallet.amount-input.view :as amount-input] | ||
[quo.foundations.colors :as colors] | ||
[test-helpers.component :as h])) | ||
|
||
(defn- render | ||
[component] | ||
(h/render-with-theme-provider component :light)) | ||
|
||
(h/describe "Amount input component" | ||
(h/test "Renders with default value" | ||
(let [text-expected 0] | ||
(render [amount-input/view {:init-value text-expected}]) | ||
(h/is-truthy (h/query-by-label-text :amount-input)) | ||
(h/is-equal (oops/oget (h/get-by-label-text :amount-input) "props" "value") | ||
(str text-expected)))) | ||
|
||
(h/test "When the value = minimum dec button is disabled" | ||
(render [amount-input/view | ||
{:init-value 0 | ||
:min-value 0}]) | ||
(h/is-truthy | ||
(oops/oget (h/get-by-label-text :amount-input-dec-button) "props" "accessibilityState" "disabled"))) | ||
|
||
(h/test "When the value = maximum inc button is disabled" | ||
(render [amount-input/view | ||
{:init-value 100 | ||
:max-value 100}]) | ||
(h/is-truthy | ||
(oops/oget (h/get-by-label-text :amount-input-inc-button) "props" "accessibilityState" "disabled"))) | ||
|
||
(h/test "Renders the error state" | ||
(render [amount-input/view {:status :error}]) | ||
(h/is-equal (colors/resolve-color :danger :light) | ||
(oops/oget (h/get-by-label-text :amount-input) "props" "style" "color"))) | ||
|
||
(h/test "on-change-text function is fired" | ||
(let [on-change-text (h/mock-fn)] | ||
(render [amount-input/view {:on-change-text on-change-text}]) | ||
(h/fire-event :change-text (h/get-by-label-text :amount-input) "100") | ||
(h/was-called on-change-text)))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
(ns quo.components.wallet.amount-input.schema) | ||
|
||
(def ?schema | ||
[:=> | ||
[:catn | ||
[:props | ||
[:map {:closed true} | ||
[:status {:optional true} [:enum :default :error]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. usually if a prop is optional, we tend to add :maybe to it to assure that nil will also pass the schema check. This would become: [:status {:optional true} [:maybe [:enum :default :error]]]
|
||
[:theme :schema.common/theme] | ||
[:on-change-text {:optional true} [:maybe fn?]] | ||
[:container-style {:optional true} [:maybe :map]] | ||
[:auto-focus {:optional true} [:maybe :boolean]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this repo, we try to suffix with a question mark if the value is a boolean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I've missed that, thanks! |
||
[:min-value {:optional true} [:maybe :int]] | ||
[:max-value {:optional true} [:maybe :int]] | ||
[:return-key-type {:optional true} [:maybe :keyword]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we know ahead of time the possible key-types, this could be also an enum. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will be the list of all of the TextInput's returnKeyType param values. Should I just list them all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i guess this should be done for the schema of the text-input rather than here, but later when we get to spec-ing the rn components. |
||
[:init-value {:optional true} [:maybe :int]]]]] | ||
:any]) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
(ns quo.components.wallet.amount-input.style | ||
(:require | ||
[quo.foundations.colors :as colors])) | ||
|
||
(def container | ||
{:flex-direction :row | ||
:justify-content :center | ||
:align-items :center}) | ||
|
||
(def input-container {:flex 1}) | ||
|
||
(defn input-text | ||
[theme type] | ||
{:padding 0 | ||
:color (if (= type :error) | ||
(colors/resolve-color :danger theme) | ||
(colors/theme-colors colors/neutral-100 colors/white theme))}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
(ns quo.components.wallet.amount-input.view | ||
(:require | ||
[quo.components.buttons.button.view :as button] | ||
[quo.components.markdown.text :as text] | ||
[quo.components.wallet.amount-input.schema :as amount-input.schema] | ||
[quo.components.wallet.amount-input.style :as style] | ||
[quo.theme :as quo.theme] | ||
[react-native.core :as rn] | ||
[reagent.core :as reagent] | ||
[schema.core :as schema])) | ||
|
||
(defn- amount-button | ||
[{:keys [theme accessibility-label disabled? icon on-press]}] | ||
[button/button | ||
{:icon-only? true | ||
:theme theme | ||
:disabled? disabled? | ||
:type :outline | ||
:accessibility-label accessibility-label | ||
:size 32 | ||
:on-press on-press} | ||
icon]) | ||
|
||
(defn- process-amount | ||
[input-value min-value max-value] | ||
(let [parsed-input-value (parse-double input-value)] | ||
(cond | ||
(nil? parsed-input-value) min-value | ||
(>= input-value max-value) max-value | ||
(<= input-value min-value) min-value | ||
:else parsed-input-value))) | ||
|
||
(defn- view-internal | ||
[{:keys [on-change-text | ||
auto-focus | ||
init-value | ||
return-key-type | ||
container-style] | ||
:or {auto-focus false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As much as I like to see default values with destructuring, in status-mobile this has been unreliable. Too often callers pass nil values because values can be wrapped in a The thing is that the caller will expect the component to deal with nils as if it means "no value passed, use defaults". But because this function is setting default values via destructuring, nils will flow in and no default values will be used. So unfortunately in terms of code style, I have to suggest the most reliable solution, which is to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense to me. Would be nice to have it here as it is an important rule to follow, and from what I've seen a lot of code in the codebase is using defaulting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in this commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the feedback. This guideline should definitely live up there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's very useful @nazariifenii. Looks good o/ |
||
init-value 0 | ||
return-key-type :done}}] | ||
(let [value (reagent/atom init-value)] | ||
(fn [{:keys [theme status min-value max-value] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't repeated some args from the outer function. For example, this component won't be reactive to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I've done this intentionally, as some props, like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in this commit. |
||
:or {status :default | ||
min-value 0 | ||
max-value 999999999}}] | ||
[rn/view | ||
{:style (merge style/container container-style)} | ||
[amount-button | ||
{:theme theme | ||
:accessibility-label :amount-input-dec-button | ||
:icon :i/remove | ||
:on-press #(swap! value dec) | ||
:disabled? (>= min-value @value)}] | ||
[rn/view {:style style/input-container} | ||
[rn/text-input | ||
{:style | ||
(text/text-style | ||
{:size :heading-1 | ||
:weight :semi-bold | ||
:align :center | ||
:style (style/input-text theme status)}) | ||
:accessibility-label :amount-input | ||
:editable true | ||
:auto-focus auto-focus | ||
:value (str @value) | ||
:keyboard-appearance (quo.theme/theme-value :light :dark theme) | ||
:return-key-type return-key-type | ||
:input-mode :numeric | ||
:on-change-text (fn [input-value] | ||
(let [processed-amount (process-amount input-value min-value max-value)] | ||
(reset! value processed-amount) | ||
(when on-change-text | ||
(on-change-text processed-amount)) | ||
(reagent/flush)))}]] ;; Fixes the input flickering issue when typing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in this commit. |
||
[amount-button | ||
{:theme theme | ||
:icon :i/add | ||
:accessibility-label :amount-input-inc-button | ||
:on-press #(swap! value inc) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For components with high re-render rates such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in this commit. |
||
:disabled? (>= @value max-value)}]]))) | ||
|
||
(def view | ||
(quo.theme/with-theme | ||
(schema/instrument #'view-internal amount-input.schema/?schema))) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
(ns status-im.contexts.preview.quo.wallet.amount-input | ||
(:require | ||
[quo.core :as quo] | ||
[reagent.core :as reagent] | ||
[status-im.contexts.preview.quo.preview :as preview])) | ||
|
||
(def descriptor | ||
[{:key :max-value | ||
:type :number} | ||
{:key :min-value | ||
:type :number} | ||
{:key :init-value | ||
:type :number} | ||
{:type :select | ||
:key :status | ||
:options [{:key :default} | ||
{:key :error}]}]) | ||
|
||
(defn view | ||
[] | ||
(let [state (reagent/atom {:max-value 10000 | ||
:min-value 0 | ||
:init-value 1 | ||
:status :default})] | ||
(fn [] | ||
[preview/preview-container | ||
{:state state | ||
:descriptor descriptor} | ||
[quo/amount-input @state]]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilmotta, @clauxx can you check this schema please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @ilmotta and @clauxx, I just wanted to ensure the request was not lost.
Thanks in advance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for reminding, would've missed it 👍