-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Improve handling of decimal points and trailing zeroes in numbers #1183
Conversation
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.
Looks great!
Also, I believe that with your changes, we can now drastically simplify the asNumber
logic. I've made those changes in a separate branch and made a PR -- #1191 -- do let me know if you think I'm missing something and we actually do need this logic.
src/components/fields/NumberField.js
Outdated
// Cache the original value in component state | ||
this.setState({ lastValue: value }); | ||
|
||
// Check that the value is a string (if the widget used is a select (due to |
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.
@LucianBuzzo can you fix the parentheses here?
Also, what is the scenario, exactly, in which there's a select with type=number with an option that has a trailing decimal point or multiple zeroes? Won't the schema never have these? Maybe if you can add a test for this it would be clearer.
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.
@epicfaace This logic needs to be here, because as you type in a value such as 0.001
each keystroke will trigger a change event, so the input will have to handle inputs like 0.00
as the user is typing. Additionally, you may have someone copy and paste values like 201.00
into the form, which is a valid number that you would expect to be processed correctly.
12cfc74
to
e0b653f
Compare
@epicfaace I've addressed you're review comments. If it's alright with you I'd like to merge this PR and then review the changes to |
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.
Looks good. Few things I noticed though:
When I copy and paste "201.00", the value in formData becomes a string "201.00", not the number 201.
When I enter in "0.001", the number input shows .001 instead of 0.001.
I think you're right, this is good to merge, we could fix these issues in a later PR.
@epicfaace I couldn't reproduce the issue with pasting the value "201.00", could you post the schema you used to test it? |
39dfeea
to
5913047
Compare
@LucianBuzzo I can't seem to reproduce the "201.00" issue, so I probably was using the wrong version earlier. Other issues:
|
@epicfaace EDIT I worked on this and got it working nicely |
5913047
to
2534751
Compare
Looks good! Will merge. |
…sf-team#1183) Connects to rjsf-team#674 rjsf-team#958 Change-type: patch Signed-off-by: Lucian <lucian.buzzo@gmail.com>
Fixes #674, fixes #958, fixes #966
Change-type: patch
Signed-off-by: Lucian lucian.buzzo@gmail.com
Checklist
npm run cs-format
on my branch to conform my code to prettier coding style