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

FloatField use NumberInput by default #679

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Demetriex
Copy link

FloatField should use NumberInput #677

@whb07
Copy link
Contributor

whb07 commented Feb 27, 2021

Hi @Demetriex , this looks good! A couple things to add perhaps is:

Add some tests verifying the behavior:

  • When the "step" property is not defined as "any" or a specific float, then the field doesn't pass validation for that specific float. The HTML input itself when calling input.checkValidity() wont report true unless its "any" or a specific float up to the defined precision, i.e - step="0.01" validates for "1.10" but not for "1.234"

I also left some comments on the changes for you to take a look at.

Copy link
Contributor

@whb07 whb07 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. Just a couple more checks and validations to follow the spec.

@@ -818,7 +818,7 @@ class FloatField(Field):
is ignored and will not be accepted as a value.
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation for this class needs to be updated

@@ -13,9 +13,11 @@ def test_float_field():
form = F(DummyPostData(a=["v"], b=["-15.0"]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to test the "step" values association to the float input data. So a step of "any" is good with "1.234" but a step of "0.1" should not validate a "1.234" input.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for your input, it looks like this related to this issue #658.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants