fix(forms): support friendlier disabled binding in ngModel #11736

Merged
merged 1 commit into from Sep 20, 2016

Conversation

Projects
None yet
5 participants
@kara
Contributor

kara commented Sep 19, 2016

Fixes unbound disabled attributes and 'false' with ngModel.

+ it('should mark as disabled properly', fakeAsync(() => {
+ ngModel.ngOnChanges({isDisabled: new SimpleChange('', undefined)});
+ tick();
+ expect(ngModel.control.disabled).toEqual(false);

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

replace the 3 lines with expectControlDisabled('', undefined, false); ?

@vicb

vicb Sep 20, 2016

Member

replace the 3 lines with expectControlDisabled('', undefined, false); ?

This comment has been minimized.

@kara

kara Sep 20, 2016

Contributor

Hmm, I can see the savings in terms of DRY-ness, but from the suggested function call it's not immediately obvious what you're testing (and what the relationship is between the three values). My inclination is that it's more readable as is.

@kara

kara Sep 20, 2016

Contributor

Hmm, I can see the savings in terms of DRY-ness, but from the suggested function call it's not immediately obvious what you're testing (and what the relationship is between the three values). My inclination is that it's more readable as is.

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

You can find a better test description / find a better function name / add a comment. But I agree in the end it's personal pref.

@vicb

vicb Sep 20, 2016

Member

You can find a better test description / find a better function name / add a comment. But I agree in the end it's personal pref.

+ });
+ const fixture = TestBed.createComponent(NgModelForm);
+ fixture.detectChanges();
+ tick();

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

do you need to both detectChanges & tick ?

@vicb

vicb Sep 20, 2016

Member

do you need to both detectChanges & tick ?

This comment has been minimized.

@kara

kara Sep 20, 2016

Contributor

Yep, both necessary

@kara

kara Sep 20, 2016

Contributor

Yep, both necessary

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

What about switching to async() and remove all the tick() then

@vicb

vicb Sep 20, 2016

Member

What about switching to async() and remove all the tick() then

This comment has been minimized.

@kara

kara Sep 20, 2016

Contributor

I actually just switched from async to fakeAsync on @vsavkin's suggestion :) I think fakeAsync is cleaner. async would require nested whenStable() calls.

@kara

kara Sep 20, 2016

Contributor

I actually just switched from async to fakeAsync on @vsavkin's suggestion :) I think fakeAsync is cleaner. async would require nested whenStable() calls.

+ });
+ const fixture = TestBed.createComponent(NgModelForm);
+ fixture.detectChanges();
+ tick();

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

What about switching to async() and remove all the tick() then

@vicb

vicb Sep 20, 2016

Member

What about switching to async() and remove all the tick() then

@@ -420,6 +420,31 @@ export function main() {
});
}));
+ it('should disable a control with unbound disabled attr', fakeAsync(() => {
+ TestBed.overrideComponent(NgModelForm, {

This comment has been minimized.

@vicb

vicb Sep 20, 2016

Member

you can write

           const fixture = TestBed.overrideComponent(NgModelForm, {
             set: {
               template: `
            <form>
             <input name="name" [(ngModel)]="name" disabled>
           </form>`,
             }
           }).createComponent(NgModelForm);
@vicb

vicb Sep 20, 2016

Member

you can write

           const fixture = TestBed.overrideComponent(NgModelForm, {
             set: {
               template: `
            <form>
             <input name="name" [(ngModel)]="name" disabled>
           </form>`,
             }
           }).createComponent(NgModelForm);

@alexeagle alexeagle merged commit 44da498 into angular:master Sep 20, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment