feat: quo2 implement amount input component #18687
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hey @nazariifenii, 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 |
J-Son89
requested review from
J-Son89,
briansztamfater,
jo-mut,
siddarthkay and
ajayesivan
J-Son89
reviewed
Jan 31, 2024
| @@ -0,0 +1,86 @@ | |||
| (ns quo.components.inputs.amount-input.view | |||
There was a problem hiding this comment.
while it's true this is an input, we follow the same hierarchy as in Figma to make the component library easier to main.
the name space here should be
quo.components.wallet.amount-input.view
same for style file and component-spec
J-Son89
reviewed
Jan 31, 2024
| @@ -0,0 +1,29 @@ | |||
| (ns status-im.contexts.preview.quo.inputs.amount-input | |||
There was a problem hiding this comment.
same here.
status-im.contexts.preview.quo.wallet.amount-input
J-Son89
reviewed
Jan 31, 2024
src/quo/core.cljs
Outdated
| @@ -281,6 +282,7 @@ | |||
| (def recovery-phrase-input quo.components.inputs.recovery-phrase.view/recovery-phrase-input) | |||
| (def search-input quo.components.inputs.search-input.view/search-input) | |||
| (def title-input quo.components.inputs.title-input.view/view) | |||
| (def amount-input quo.components.inputs.amount-input.view/view) | |||
There was a problem hiding this comment.
this will be moved with the ns updates, but please keep this file alpha sorted ( depending on what section of components it is)
J-Son89
reviewed
Jan 31, 2024
|
|
||
| (def view | ||
| (quo.theme/with-theme | ||
| (schema/instrument #'view-internal amount-input.schema/?schema))) |
J-Son89
reviewed
Jan 31, 2024
| min-value 0 | ||
| max-value 999999999}}] | ||
| [rn/view | ||
| {:style (merge style/container container-style)} |
J-Son89
reviewed
Jan 31, 2024
| [theme type] | ||
| {:padding 0 | ||
| :color (if (= type :error) | ||
| colors/danger-60 |
There was a problem hiding this comment.
please use (colors/resolve-color :danger theme 60) in place of colors/danger-60
There was a problem hiding this comment.
BTW, @J-Son89, looks like the third parameter of the function is for the opacity, and according to the Figma the opacity should be 100%, so I'll remove param "60". The function will look next (colors/resolve-color :danger theme).
There was a problem hiding this comment.
Sorry yes you are correct, my bad for the confusion 😅🙏
J-Son89
reviewed
Jan 31, 2024
|
|
||
| (defn get-selection-color | ||
| [customization-color theme] | ||
| (colors/alpha |
There was a problem hiding this comment.
this should instead be -
(defn get-selection-color
[customization-color theme]
(colors/resolve-color customization-color
theme
(if platform/ios? 1 0.2)))
as resolve color handles the opacity with the 3rd parameter. The 50/60 values are also handled internally within the resolve color function so there is no need to do that there.
Jenkins BuildsClick to see older builds (53)
|
…bile into feat/amount-input
75% of end-end tests have passedFailed tests (10)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (36)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
OmarBasem
reviewed
Feb 2, 2024
| [customization-color theme] | ||
| (colors/resolve-color customization-color | ||
| theme | ||
| (if platform/ios? 1 0.2))) |
There was a problem hiding this comment.
Why is the value different for ios and Android here?
There was a problem hiding this comment.
That was a leftover from the copying of this function. The :selection-color param is probably not needed here at all. What do you think @OmarBasem?
There was a problem hiding this comment.
@J-Son89 do you know why we have different values here? 🤔
There was a problem hiding this comment.
No idea, @nazariifenii whatever is in here should be matching the designs. So if it's not needed or just some mistakingly copied code then we probably don't need it.
We will ask the design team for a review of the component so once you feel it's at a ready state for that let us know and we can ask them :)
There was a problem hiding this comment.
If you need a hand inspecting the designs feel free to ask and we can go through it
There was a problem hiding this comment.
I will remove this prop. From Figma, I can see that we don't need different customizations of input selection and cursor colors.
Also, I have access to hand inspecting the designs at Figma, so should be good, thank you!
There was a problem hiding this comment.
Removed in this PR.
…bile into feat/amount-input
J-Son89
reviewed
Feb 2, 2024
| @@ -0,0 +1,17 @@ | |||
| (ns quo.components.wallet.amount-input.schema) | |||
|
|
|||
| (def ?schema | |||
There was a problem hiding this comment.
thanks for reminding, would've missed it 👍
|
@Francesca-G - could you give this component a design review when you have time? 🙏 |
clauxx
reviewed
Feb 5, 2024
| [:catn | ||
| [:props | ||
| [:map {:closed true} | ||
| [:status {:optional true} [:enum :default :error]] |
There was a problem hiding this comment.
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]]]
| [:auto-focus {:optional true} [:maybe :boolean]] | ||
| [: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.
if we know ahead of time the possible key-types, this could be also an enum.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ilmotta
reviewed
Feb 5, 2024
| init-value | ||
| return-key-type | ||
| container-style] | ||
| :or {auto-focus false |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Fixed in this commit.
There was a problem hiding this comment.
Thank you for the feedback. This guideline should definitely live up there.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed in this commit.
| (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.
There was a problem hiding this comment.
Fixed in this commit.
| {:theme theme | ||
| :icon :i/add | ||
| :accessibility-label :amount-input-inc-button | ||
| :on-press #(swap! value inc) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Fixed in this commit.
| [: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.
In this repo, we try to suffix with a question mark if the value is a boolean.
There was a problem hiding this comment.
Sure, I've missed that, thanks!
ilmotta
approved these changes
Feb 6, 2024
|
@status-im/mobile-qa - this pr is just a design system component (quo). |
67% of end-end tests have passedFailed tests (15)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (32)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #18641
Summary
This PR implements a Quo2 Amount input component.
Review notes
Platforms
Areas that maybe impacted
None, only components preview screen
Steps to test
status: ready