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

Fix for issue #519, unnecessary check for validatable observables #529

Closed
wants to merge 1 commit into from

Conversation

brettmorien
Copy link
Contributor

Still learning how to git properly, so here's a different pull request for issue #519.

@@ -625,6 +625,74 @@ QUnit.test('Issue #44 - Validation Element - Is Invalid Test', function(assert)

});

QUnit.test('Validatable can become non-validatable', 0, function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using this syntax (0 expectations), why not add an assert at the end of the test, like assert.equal(ko.validation.utils.isValidatable(false), false); IMO, It would better describe the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an accident leaving this test behind. Removing it as redundant and incomplete.

…h validationElement and validationMessage in markup.

#519

Additional unit tests for issue #519

Undo updates to package.json

Rejiggered unit tests for issue #519 to be more thorough.

Fix unit tests.
@crissdev
Copy link
Member

Looks good. Can I merge this or you want to make further changes?

@brettmorien
Copy link
Contributor Author

Done and ready to go. Merge away.

crissdev added a commit that referenced this pull request Jan 30, 2015
validationElement and validationMessage bindings throw if observable is not validatable.
@crissdev
Copy link
Member

landed in 8409e9a

@crissdev crissdev closed this Jan 30, 2015
@brettmorien
Copy link
Contributor Author

Sweet. Thanks!

@crissdev
Copy link
Member

At this point 2.0.2 is ready. Unless there's any urgent bug to be fixed. Going to update the NuGet package also for this release - most probably on Monday.
FYI, the file names for NuGet package will change to be the same as for npm and bower. This might be a bit frustrating for some users but I think it's better do this change now than later.

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

Successfully merging this pull request may close these issues.

2 participants