Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(input): ng-required conflicts with ng-true-value #6065

Closed
wants to merge 1 commit into from
Closed

fix(input): ng-required conflicts with ng-true-value #6065

wants to merge 1 commit into from

Conversation

tameraydin
Copy link
Contributor

Before this change, overridden "isEmpty" method which is called in "requiredDirective" would return a wrong result when the parameter is given as "true". This change corrects the statement value within method.

Closes #5164

it('should allow custom enumaration even if it is required', function() {
compileInput('<input type="checkbox" ng-model="name" ng-true-value="y" ' +
'ng-false-value="n" ng-required="true">');

Copy link
Contributor

Choose a reason for hiding this comment

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

You should also test that the expected behaviour occurs when the field is empty. I also want to see lots of

expect(inputElm).toBeValid(); // or toBeInvalid(); if it makes sense

in this spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now the tests also cover validation status.

@ghost ghost assigned vojtajina and tbosch Jan 31, 2014
@tameraydin tameraydin closed this Feb 10, 2014
@tameraydin tameraydin reopened this Feb 10, 2014
@caitp
Copy link
Contributor

caitp commented Feb 11, 2014

@matsko this closes an issue assigned to you, do you want to merge this?

IMO it LGTM, although the change to $isEmpty doesn't really make sense to me, I don't see why it works to compare to "true". So that might deserve a closer look.

@matsko
Copy link
Contributor

matsko commented Feb 11, 2014

@caitp I'm not sure who assigned it to me in the first place. If you want to take it over then that works, otherwise I can review it.

@caitp
Copy link
Contributor

caitp commented Feb 11, 2014

It's already got an LGTM from me based on the tests, I just haven't looked into why it actually works, it might be dependent on what the formatter stack looks like. I expect it's probably good, though.

@tameraydin
Copy link
Contributor Author

Hi @caitp,
So as to clarify, when the input is clicked -which has true value then-, isEmpty method (triggered by ng-require) was incorrectly returning true because of strict comparison with trueValue (lets assume given a string on ng-true-value).

BTW, I see the build is not completed due to a missing package error, -but the code was successfully passed-, do I have anything to do with it?

@caitp
Copy link
Contributor

caitp commented Feb 11, 2014

I don't think you should worry about that, but you should rebase your code as the travis tests have changed, and it will make life easier when we merge this.

Before this change, overridden "isEmpty" method which is called in "requiredDirective" would return a wrong result when the parameter is given as "true". This change corrects the statement value within method.

Closes #5164
@Narretz
Copy link
Contributor

Narretz commented Oct 13, 2014

This was fixed and works in current rc / snapshot. (1.3.0-rc.5)

@Narretz Narretz closed this Oct 13, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng-required conflicts with ng-true-value
7 participants