fix for issue #430 #436
fix for issue #430 #436
Conversation
made assumption that any existing label which already has errorClass is safe to update with a new message.
| @@ -683,7 +683,7 @@ $.extend($.validator, { | |||
| label.removeClass( this.settings.validClass ).addClass( this.settings.errorClass ); | |||
|
|
|||
| // check if we have a generated label, replace the message then | |||
| if ( label.attr("generated") ) { | |||
| if ( label.attr("generated") || label.hasClass(this.settings.errorClass) ) { | |||
There was a problem hiding this comment.
Line 683 adds the errorClass to the label, so this would always be true. Could you address that, along with unit tests?
There was a problem hiding this comment.
D'oh, can't believe i missed that! Hmm, I suppose we could check whether it's valid to replace the message before the error class is added. e.g.
var okToReplace = label.attr("generated") || label.hasClass(this.settings.errorClass);
label.removeClass( this.settings.validClass ).addClass( this.settings.errorClass );
if(okToReplace) {
// do replacement
}Does that seem fair to you?
There was a problem hiding this comment.
That should work. Unit tests to the rescue!
Feel free to update this PR. Can just amend the existing commit and force-push. Or add new commits and I'll squash later.
|
Thanks for the contribution. Added an inline comment above. |
|
Would you like any further changes in a fresh branch or are you ok to accept on master? |
|
Generally PRs should come from branches, that way to can start working on something else. If you don't plan to do that anyway, just push commits to master and I'll merge from there. |
|
Finally got around to fixing previous commit and adding a unit test - completely forgot about this! This is the first time I've ever had to sync a seriously out of date fork, so apologies if anything is out of place. Happy to make any adjustments you require :) |
|
ahem :) |
|
Finally merged this. I looked at Thanks for the contribution! |
|
Thanks for that :) Sorry for the ignorance, not familiar with the codebase to understand what you wrote above! Does this mean my change wasn't necessary or that there was a better place to put the fix? My unit test still helped I hope! |
|
You can compare my commit above to your original changes. The fix was valid, it just was more complicated than necessary. |
|
Makes sense, this is why domain knowledge is valuable :) Thanks for accepting! |
Firstly, apologies if I've done something wrong here, my first effort at contributing a fix. I think I've committed against an issue, though I may have done it completely wrong!
This covers the case where the the form has failed server-side validation and so the form is sent back to the user with server-generated error labels. Currently the validation plugin will not update these labels if new user input causes client-side validation to fail. This commit fixes that.
I made the assumption that any existing label which already has the errorClass assigned is safe to update with a new message, which i believe is a safe assumption (why would any label other than a validation error label, already have the errorClass assigned?)
Thanks,
Nick