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

Min and Max attributes now rely on type attribute for comparison #324

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@codethug
Contributor

codethug commented Sep 17, 2013

When used as HTML5 attributes, min and max were comparing as strings. So an element of <input min="10" data-bind="value:myNumber"> would validate successfully for a value of 5, because the string "5" is greater than the string "10". This was understandably causing some problems.

This update changes the min and max validators so that they pay attention to the type attribute. For example, the element <input type="month" min="2010-03" data-bind="value:myMonth"> will use appropriate logic to ensure that the entered date is entered as a month and is March 2010 or later. And an element of <input type="number" min="10" data-bind="value:myNumber"> will fail validation if a value of 5 is entered.

Tests and Documentation updates included.

@stevegreatrex

This comment has been minimized.

Show comment
Hide comment
@stevegreatrex

stevegreatrex Sep 20, 2013

Contributor

This looks great!

Contributor

stevegreatrex commented Sep 20, 2013

This looks great!

@codethug

This comment has been minimized.

Show comment
Hide comment
@codethug

codethug Sep 30, 2013

Contributor

What is the next step in having this pulled into Knockout-Contrib?

Contributor

codethug commented Sep 30, 2013

What is the next step in having this pulled into Knockout-Contrib?

@stevegreatrex

This comment has been minimized.

Show comment
Hide comment
@stevegreatrex

stevegreatrex Sep 30, 2013

Contributor

I've had a quick read through the code and whilst it's generally good I think it would benefit from some tidying up in the min & max validators where it looks (admittedly after only a cursory read-through) like there is some scope for refactoring to reduce the amount of duplicate/very similar code. What do you think?

Contributor

stevegreatrex commented Sep 30, 2013

I've had a quick read through the code and whilst it's generally good I think it would benefit from some tidying up in the min & max validators where it looks (admittedly after only a cursory read-through) like there is some scope for refactoring to reduce the amount of duplicate/very similar code. What do you think?

@codethug

This comment has been minimized.

Show comment
Hide comment
@codethug

codethug Oct 1, 2013

Contributor

Steve, I have refactored the min and max validators to reduce the duplicate code. The only difference between them was that one used <= and the other used >=. I'm now calling 'min' from 'max', passing in a boolean parameter to indicate that the comparison should be inverted.

Please review and let me know if anything else needs to be cleaned up.

Contributor

codethug commented Oct 1, 2013

Steve, I have refactored the min and max validators to reduce the duplicate code. The only difference between them was that one used <= and the other used >=. I'm now calling 'min' from 'max', passing in a boolean parameter to indicate that the comparison should be inverted.

Please review and let me know if anything else needs to be cleaned up.

@stevegreatrex

This comment has been minimized.

Show comment
Hide comment
@stevegreatrex

stevegreatrex Oct 1, 2013

Contributor

This is looking better but it seems a bit strange to call max from min (or vice versa)...instead, what about creating a validator factory function that performs that switching? I've pushed your changes and a possible implementation of this to the codethug-minmax branch on here - let me know what you think.

90eed14

I also noticed that quite a few of the tests are failing in IE11. Chrome was working fine but I haven't had a chance to test in other browsers. What have you been using to run the tests?

I feel like we could make the massive switch statement a bit nicer as well, but I don't have time to put something together just now.

We're making progress though!

Contributor

stevegreatrex commented Oct 1, 2013

This is looking better but it seems a bit strange to call max from min (or vice versa)...instead, what about creating a validator factory function that performs that switching? I've pushed your changes and a possible implementation of this to the codethug-minmax branch on here - let me know what you think.

90eed14

I also noticed that quite a few of the tests are failing in IE11. Chrome was working fine but I haven't had a chance to test in other browsers. What have you been using to run the tests?

I feel like we could make the massive switch statement a bit nicer as well, but I don't have time to put something together just now.

We're making progress though!

@codethug

This comment has been minimized.

Show comment
Hide comment
@codethug

codethug Oct 2, 2013

Contributor

Good idea on the factory. I wasn't quite sure what to do with the actual implementation. I'll take a closer look at your code.

As for IE11 tests, I don't have IE11 available, but I tried running the tests on IE10 and I saw 2-3 assertions failing (sometimes 2, sometimes 3) for a single test related to HTML5InputAttributes. These tests were failing even without the changes in this pull request. As such, I've created a separate pull request to fix that test at #331.

Are you seeing other tests fail other than that with IE11? And do these tests run fine before my min/max changes are introduced? If so, I'll work up IE11 on a VM so that I can reproduce and resolve the problem.

Contributor

codethug commented Oct 2, 2013

Good idea on the factory. I wasn't quite sure what to do with the actual implementation. I'll take a closer look at your code.

As for IE11 tests, I don't have IE11 available, but I tried running the tests on IE10 and I saw 2-3 assertions failing (sometimes 2, sometimes 3) for a single test related to HTML5InputAttributes. These tests were failing even without the changes in this pull request. As such, I've created a separate pull request to fix that test at #331.

Are you seeing other tests fail other than that with IE11? And do these tests run fine before my min/max changes are introduced? If so, I'll work up IE11 on a VM so that I can reproduce and resolve the problem.

@stevegreatrex

This comment has been minimized.

Show comment
Hide comment
@stevegreatrex

stevegreatrex Oct 2, 2013

Contributor

I was seeing quite a few tests failing (around 12 IIRC) but many of those were the new ones that had been added.

See if you can get the new tests running then submit a new PR to the new branch and I'll merge everything in.

Contributor

stevegreatrex commented Oct 2, 2013

I was seeing quite a few tests failing (around 12 IIRC) but many of those were the new ones that had been added.

See if you can get the new tests running then submit a new PR to the new branch and I'll merge everything in.

@codethug

This comment has been minimized.

Show comment
Hide comment
@codethug

codethug Oct 30, 2013

Contributor

Closing this PR because Knockout-Validation rearranged its folder structure, and updating this with another commit would make this PR have all kinds of crazy commits.

This work is continued at #355 so that we can have fewer commits that will cleanly merge with the latest master for Knockout-Validation.

Contributor

codethug commented Oct 30, 2013

Closing this PR because Knockout-Validation rearranged its folder structure, and updating this with another commit would make this PR have all kinds of crazy commits.

This work is continued at #355 so that we can have fewer commits that will cleanly merge with the latest master for Knockout-Validation.

@codethug codethug closed this Oct 30, 2013

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