-
Notifications
You must be signed in to change notification settings - Fork 984
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
[7105] Feature - Adding error for a field defined with range #8229
[7105] Feature - Adding error for a field defined with range #8229
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (27)
|
(defn input [{:keys [keyboard-type style error on-change change-delay placeholder placeholder-text-color selection-color | ||
(defn- in-range | ||
;; Determines whether a number is in a range | ||
[n, start, end] |
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.
Commas are a bit weird here :)
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.
No more commas at all 👍
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.
I still see them?
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.
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.
they are still here
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.
commas still here
@@ -25,26 +28,44 @@ | |||
(js/clearTimeout id)) | |||
(reset! current (js/setTimeout #(on-input-change-text on-change value) delay))) | |||
|
|||
(defn input [{:keys [keyboard-type style error on-change change-delay placeholder placeholder-text-color selection-color | |||
(defn- in-range | |||
;; Determines whether a number is in a range |
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.
Should be defined as function doc?
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.
I think no need comments at all, the function is pretty simple and self explained
(cond | ||
(nil? n) true | ||
(and (nil? start) (nil? end)) true | ||
(= n "") true |
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.
Can be combined with nil check into string/empty?
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.
changed 👍
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.
it could be simplest form
(or (string/blank? n) (and (nil? start) (nil? end)) (and (< n end) (> n start)) )
[tooltip/tooltip error (styles/error error)])]) | ||
|
||
(let [input-range (reagent/atom nil)] | ||
(fn [] |
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.
Why the fn here?
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.
@jeluard I tried to do in different ways, and only works when there's a anonymous function to handle. In some tutorials about atoms and event handling using clojure, reagent and react it was using this approach.
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.
What didn't work w/o it?
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.
@jeluard I think it is related to event handling with reagent, it needs to re-render once the atom change with the updated values.
You can see here:
https://github.com/status-im/status-react/blob/develop/src/status_im/ui/screens/wallet/components/views.cljs#L229
It is a similar situation where fn
is used, I there's many of those inside the project.
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.
But what didn't work exactly? Not clear from your comments
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.
Sorry @jeluard, without fn
wrapped on the component when using reagent/atom
, the input don't update the atom value and didn't check on on-change
for the in-range
function, so nothing happens if I get rid of the fn
inside. This means the validation and the input error don't appear and nothing happens.
[status-im.ui.components.text-input.styles :as styles] | ||
[status-im.extensions.capacities.map :as map-component])) | ||
(:require | ||
[reagent.core :as reagent] |
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.
We usually use previous format
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.
I don't know why these spaces was inserted, but I Solved now 👍
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.
for some reason i can't see changes in the code, maybe something wrong with commits. could you please squash commits and rebase onto develop, thanks
(defn input [{:keys [keyboard-type style error on-change change-delay placeholder placeholder-text-color selection-color | ||
(defn- in-range | ||
;; Determines whether a number is in a range | ||
[n, start, end] |
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.
they are still here
(cond | ||
(nil? n) true | ||
(and (nil? start) (nil? end)) true | ||
(= n "") true |
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.
it could be simplest form
(or (string/blank? n) (and (nil? start) (nil? end)) (and (< n end) (> n start)) )
f622566
to
ea81090
Compare
please try now @flexsurfer , there's a only commit only now 👍 |
Hey @alexanmtz could you please rebase onto |
c6e92f9
to
e98a964
Compare
Done 👍 @Serhy |
@alexanmtz do you have your extension posted on ipfs so you can share a link? |
98% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (48)Click to expand
|
@asemiankevich https://get.status.im/extension/ipfs@QmShrYmDYGAicPRYkqfq98Aipee5oQ1op5r9ZCwGjJWF7g/ Is set to a min value of 1 and max of 8 to the input test, that would be available on wallet settings. |
The code of the extension is on the PR:
|
it fails after i install it in status |
Try this: Or try to create the extension yourself with the code I provided. |
Looks good on iOS and Android.
Thanks, @alexanmtz ! |
…xtensions Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
e98a964
to
287e097
Compare
Fixes #7105
Summary
Support input range validation with
min
andmax
propertiesReview notes
Testing notes
Platforms
Areas that maybe impacted
Functional
Non-functional
Steps to test
Add the following extension to test
status: ready