Skip to content

Commit

Permalink
refactor(select): simplify option sync and more robust unit tests (an…
Browse files Browse the repository at this point in the history
…gular#7807)

* Currently the select sets the `multiple` and `disableRipple` values on its options manually. In order to avoid "changed after checked" errors we wrap the calls in promises, however this also forces us to have `async` tests which can time out if the browser is out of focus. These changes add a provider that allows for the options to take the value directly from the select.
* Refactors some unit tests that have been timing out in Firefox to run in the `fakeAsync` zone instead of the `async` one.
  • Loading branch information
crisbeto authored and tinayuangao committed Oct 18, 2017
1 parent 26bbeb2 commit 4fc3a0b
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 158 deletions.
2 changes: 2 additions & 0 deletions src/cdk/overlay/position/connected-position-strategy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,8 @@ describe('ConnectedPositionStrategy', () => {

it('should allow for the fallback positions to specify their own offsets', () => {
originElement.style.bottom = '0';
originElement.style.left = '50%';
originElement.style.position = 'fixed';
originRect = originElement.getBoundingClientRect();
strategy = positionBuilder
.connectedTo(
Expand Down
16 changes: 1 addition & 15 deletions src/lib/core/option/option.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('MatOption component', () => {
});

it('should show ripples by default', () => {
expect(optionInstance.disableRipple).toBe(false, 'Expected ripples to be enabled by default');
expect(optionInstance.disableRipple).toBeFalsy('Expected ripples to be enabled by default');
expect(optionNativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected no ripples to show up initially');

Expand All @@ -54,20 +54,6 @@ describe('MatOption component', () => {
.toBe(0, 'Expected no ripples to show up after click on a disabled option.');
});

it('should not show ripples if the ripples are disabled using disableRipple', () => {
expect(optionNativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected no ripples to show up initially');

optionInstance.disableRipple = true;
fixture.detectChanges();

dispatchFakeEvent(optionNativeElement, 'mousedown');
dispatchFakeEvent(optionNativeElement, 'mouseup');

expect(optionNativeElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected no ripples to show up after click when ripples are disabled.');
});

});

});
Expand Down
46 changes: 25 additions & 21 deletions src/lib/core/option/option.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
Output,
QueryList,
ViewEncapsulation,
InjectionToken,
Inject,
} from '@angular/core';
import {MatOptgroup} from './optgroup';

Expand All @@ -33,6 +35,22 @@ export class MatOptionSelectionChange {
constructor(public source: MatOption, public isUserInput = false) { }
}

/**
* Describes a parent component that manages a list of options.
* Contains properties that the options can inherit.
* @docs-private
*/
export interface MatOptionParentComponent {
disableRipple?: boolean;
multiple?: boolean;
}

/**
* Injection token used to provide the parent component to options.
*/
export const MAT_OPTION_PARENT_COMPONENT =
new InjectionToken<MatOptionParentComponent>('MAT_OPTION_PARENT_COMPONENT');

/**
* Single option inside of a `<mat-select>` element.
*/
Expand Down Expand Up @@ -60,24 +78,13 @@ export class MatOptionSelectionChange {
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class MatOption {
private _selected: boolean = false;
private _active: boolean = false;
private _multiple: boolean = false;
private _disableRipple: boolean = false;

/** Whether the option is disabled. */
private _disabled: boolean = false;

private _id: string = `mat-option-${_uniqueIdCounter++}`;
private _selected = false;
private _active = false;
private _disabled = false;
private _id = `mat-option-${_uniqueIdCounter++}`;

/** Whether the wrapping component is in multiple selection mode. */
get multiple() { return this._multiple; }
set multiple(value: boolean) {
if (value !== this._multiple) {
this._multiple = value;
this._changeDetectorRef.markForCheck();
}
}
get multiple() { return this._parent && this._parent.multiple; }

/** The unique ID of the option. */
get id(): string { return this._id; }
Expand All @@ -94,18 +101,15 @@ export class MatOption {
set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }

/** Whether ripples for the option are disabled. */
get disableRipple() { return this._disableRipple; }
set disableRipple(value: boolean) {
this._disableRipple = value;
this._changeDetectorRef.markForCheck();
}
get disableRipple() { return this._parent && this._parent.disableRipple; }

/** Event emitted when the option is selected or deselected. */
@Output() onSelectionChange = new EventEmitter<MatOptionSelectionChange>();

constructor(
private _element: ElementRef,
private _changeDetectorRef: ChangeDetectorRef,
@Optional() @Inject(MAT_OPTION_PARENT_COMPONENT) private _parent: MatOptionParentComponent,
@Optional() public readonly group: MatOptgroup) {}

/**
Expand Down
Loading

0 comments on commit 4fc3a0b

Please sign in to comment.