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

Improve handling of decimal points and trailing zeroes in numbers #1183

Merged
merged 1 commit into from Feb 26, 2019

Conversation

LucianBuzzo
Copy link
Collaborator

@LucianBuzzo LucianBuzzo commented Feb 15, 2019

Fixes #674, fixes #958, fixes #966

Change-type: patch
Signed-off-by: Lucian lucian.buzzo@gmail.com

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

test/NumberField_test.js Outdated Show resolved Hide resolved
test/NumberField_test.js Outdated Show resolved Hide resolved
Copy link
Member

@epicfaace epicfaace left a 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/widgets/BaseInput.js Show resolved Hide resolved
src/components/widgets/BaseInput.js Show resolved Hide resolved
// 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
Copy link
Member

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.

Copy link
Collaborator Author

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.

@LucianBuzzo
Copy link
Collaborator Author

@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 asNumber independently.

Copy link
Member

@epicfaace epicfaace left a 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.

src/components/fields/NumberField.js Outdated Show resolved Hide resolved
@LucianBuzzo
Copy link
Collaborator Author

@epicfaace I couldn't reproduce the issue with pasting the value "201.00", could you post the schema you used to test it?
The issue with "0.001" is repeatable and a bit of a problem, I'll add some tests and fix it.

@LucianBuzzo LucianBuzzo force-pushed the 674-number-handling branch 4 times, most recently from 39dfeea to 5913047 Compare February 26, 2019 10:22
@epicfaace
Copy link
Member

@LucianBuzzo I can't seem to reproduce the "201.00" issue, so I probably was using the wrong version earlier.

Other issues:

  • I can't type in ".00"
  • If I type in ".01", the number input only ends up with a "1"
  • I can't paste in ".00" or "0.00"

@LucianBuzzo
Copy link
Collaborator Author

LucianBuzzo commented Feb 26, 2019

@epicfaace What do you think of automatically prefix . with a 0 so that if .00 is added it automatically becomes 0.00?

EDIT I worked on this and got it working nicely

Connects to #674 #958

Change-type: patch
Signed-off-by: Lucian <lucian.buzzo@gmail.com>
@epicfaace
Copy link
Member

Looks good! Will merge.

@epicfaace epicfaace merged commit aebfab9 into master Feb 26, 2019
CodeGains pushed a commit to CodeGains/react-jsonschema-form that referenced this pull request Mar 5, 2019
…sf-team#1183)

Connects to rjsf-team#674 rjsf-team#958

Change-type: patch
Signed-off-by: Lucian <lucian.buzzo@gmail.com>
@epicfaace epicfaace mentioned this pull request Apr 23, 2019
2 tasks
@epicfaace epicfaace deleted the 674-number-handling branch May 20, 2020 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants