Skip to content
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

Merged
merged 23 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6eea6c5
Added amount input component
nazariifenii Jan 31, 2024
d221e97
Run make lint
nazariifenii Jan 31, 2024
1b29ad9
Moved amount input to wallet ns
nazariifenii Feb 1, 2024
334744f
Fixed amount input component styles
nazariifenii Feb 1, 2024
8fcc03d
Fixed input flickering issue
nazariifenii Feb 1, 2024
0087420
Run make lint-fix
nazariifenii Feb 1, 2024
d46daf6
Fixed component spec test
nazariifenii Feb 1, 2024
de3ff53
Updated component props
nazariifenii Feb 1, 2024
73bda8e
Merge branch 'develop' into feat/amount-input
nazariifenii Feb 1, 2024
797e696
Fixed input text error color opacity
nazariifenii Feb 1, 2024
17e594a
Merge branch 'feat/amount-input' of github.com:nazariifenii/status-mo…
nazariifenii Feb 1, 2024
80e0070
Fixed components test
nazariifenii Feb 1, 2024
409fef2
Update src/quo/components/wallet/amount_input/component_spec.cljs
nazariifenii Feb 1, 2024
8430a37
Removed selection-color input prop
nazariifenii Feb 2, 2024
044a717
Merge branch 'feat/amount-input' of github.com:nazariifenii/status-mo…
nazariifenii Feb 2, 2024
d6acecc
Fixed amount input schema
nazariifenii Feb 6, 2024
cb3d246
Refactored code
nazariifenii Feb 6, 2024
84dca31
Merge branch 'develop' into feat/amount-input
nazariifenii Feb 8, 2024
23f805a
Merge branch 'develop' into feat/amount-input
nazariifenii Feb 8, 2024
c9d8e4a
Merge branch 'develop' into feat/amount-input
nazariifenii Feb 8, 2024
9eb522e
Merge branch 'develop' into feat/amount-input
nazariifenii Feb 9, 2024
64683ab
Merge branch 'develop' into feat/amount-input
J-Son89 Feb 9, 2024
ecd3a89
Merge branch 'develop' into feat/amount-input
J-Son89 Feb 12, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added resources/images/icons2/20x20/[email protected]
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
43 changes: 43 additions & 0 deletions src/quo/components/wallet/amount_input/component_spec.cljs
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))))
17 changes: 17 additions & 0 deletions src/quo/components/wallet/amount_input/schema.cljs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
(ns quo.components.wallet.amount-input.schema)

(def ?schema
Copy link
Contributor

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?

Copy link
Contributor Author

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!

Copy link
Member

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 👍

[:=>
[:catn
[:props
[:map {:closed true}
[:status {:optional true} [:enum :default :error]]
Copy link
Member

Choose a reason for hiding this comment

The 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]]
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]]
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?
Also, it may vary from one RN version to another. Very rarely, but it is possible.

Copy link
Member

@clauxx clauxx Feb 6, 2024

Choose a reason for hiding this comment

The 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])
17 changes: 17 additions & 0 deletions src/quo/components/wallet/amount_input/style.cljs
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))})
85 changes: 85 additions & 0 deletions src/quo/components/wallet/amount_input/view.cljs
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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 when macro for example.

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 (or value some-default-value) in a let.

Copy link
Contributor Author

@nazariifenii nazariifenii Feb 6, 2024

Choose a reason for hiding this comment

The 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 :or when destructuring instead of (or value def-value). I'm happy to contribute to the guidelines update if needed!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in this commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. This guideline should definitely live up there.

Copy link
Contributor Author

@nazariifenii nazariifenii Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ilmotta I had some time and created the PR with the doc update #18731. Do you think it makes sense to do this?
If yes, I can make the PR Ready for Review.
I've used some of your sentences, thank you for that.

Copy link
Contributor

Choose a reason for hiding this comment

The 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]
Copy link
Contributor

Choose a reason for hiding this comment

The 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 auto-focus unless it's fully re-mounted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've done this intentionally, as some props, like auto-focus? and return-key-type are unlikely to be changed when the component is mounted. But I'm happy to apply your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For components with high re-render rates such as view, we have to be careful with regenerating anonymous functions and passing them downstream. This forces Reagent to do more work than it needs to. Better is to extract as much as possible to let bindings on the mount phase (alongside value).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)))
2 changes: 2 additions & 0 deletions src/quo/core.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@
quo.components.wallet.account-overview.view
quo.components.wallet.account-permissions.view
quo.components.wallet.address-text.view
quo.components.wallet.amount-input.view
quo.components.wallet.confirmation-progress.view
quo.components.wallet.keypair.view
quo.components.wallet.network-amount.view
Expand Down Expand Up @@ -412,6 +413,7 @@
(def account-overview quo.components.wallet.account-overview.view/view)
(def account-permissions quo.components.wallet.account-permissions.view/view)
(def address-text quo.components.wallet.address-text.view/view)
(def amount-input quo.components.wallet.amount-input.view/view)
(def confirmation-propgress quo.components.wallet.confirmation-progress.view/view)
(def keypair quo.components.wallet.keypair.view/view)
(def network-amount quo.components.wallet.network-amount.view/view)
Expand Down
1 change: 1 addition & 0 deletions src/quo/core_spec.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
quo.components.wallet.account-origin.component-spec
quo.components.wallet.account-overview.component-spec
quo.components.wallet.account-permissions.component-spec
quo.components.wallet.amount-input.component-spec
quo.components.wallet.confirmation-progress.component-spec
quo.components.wallet.keypair.component-spec
quo.components.wallet.network-amount.component-spec
Expand Down
3 changes: 3 additions & 0 deletions src/status_im/contexts/preview/quo/main.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@
[status-im.contexts.preview.quo.wallet.account-overview :as
account-overview]
[status-im.contexts.preview.quo.wallet.account-permissions :as account-permissions]
[status-im.contexts.preview.quo.wallet.amount-input :as amount-input]
[status-im.contexts.preview.quo.wallet.confirmation-progress :as
confirmation-progress]
[status-im.contexts.preview.quo.wallet.keypair :as keypair]
Expand Down Expand Up @@ -490,6 +491,8 @@
:component account-overview/view}
{:name :account-permissions
:component account-permissions/view}
{:name :amount-input
:component amount-input/view}
{:name :confirmation-progress
:component confirmation-progress/view}
{:name :keypair :component keypair/view}
Expand Down
29 changes: 29 additions & 0 deletions src/status_im/contexts/preview/quo/wallet/amount_input.cljs
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]])))