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: consistently coerce boolean and number properties to the correct type #7283

Merged
merged 7 commits into from
Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions src/lib/datepicker/datepicker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,22 @@ export class MatDatepicker<D> implements OnDestroy {
* Whether the calendar UI is in touch mode. In touch mode the calendar opens in a dialog rather
* than a popup and elements have more padding to allow for bigger touch targets.
*/
@Input() touchUi = false;
@Input()
get touchUi(): boolean {
return this._touchUi;
}
set touchUi(value: boolean) {
this._touchUi = coerceBooleanProperty(value);
}
private _touchUi = false;

/** Whether the datepicker pop-up should be disabled. */
@Input()
get disabled() {
get disabled(): boolean {
return this._disabled === undefined && this._datepickerInput ?
this._datepickerInput.disabled : this._disabled;
}
set disabled(value: any) {
set disabled(value: boolean) {
const newValue = coerceBooleanProperty(value);

if (newValue !== this._disabled) {
Expand Down Expand Up @@ -224,7 +231,7 @@ export class MatDatepicker<D> implements OnDestroy {
}
}

ngOnDestroy() {
ngOnDestroy(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to add types to lifecycle methods

this.close();
this._inputSubscription.unsubscribe();
this._disabledChange.complete();
Expand Down
7 changes: 5 additions & 2 deletions src/lib/expansion/accordion-item.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from '@angular/core';
import {UniqueSelectionDispatcher} from '@angular/material/core';
import {CdkAccordion} from './accordion';
import {coerceBooleanProperty} from '@angular/cdk/coercion';

/** Used to generate unique ID for each expansion panel. */
let nextId = 0;
Expand All @@ -38,8 +39,10 @@ export class AccordionItem implements OnDestroy {

/** Whether the AccordionItem is expanded. */
@Input()
get expanded(): boolean { return this._expanded; }
set expanded(expanded: boolean) {
get expanded(): any { return this._expanded; }
set expanded(expanded: any) {
expanded = coerceBooleanProperty(expanded);

// Only emit events and update the internal value if the value changes.
if (this._expanded !== expanded) {
this._expanded = expanded;
Expand Down
11 changes: 10 additions & 1 deletion src/lib/expansion/expansion-panel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {CanDisable, mixinDisabled, UniqueSelectionDispatcher} from '@angular/mat
import {Subject} from 'rxjs/Subject';
import {MatAccordion} from './accordion';
import {AccordionItem} from './accordion-item';
import {coerceBooleanProperty} from '@angular/cdk/coercion';


// Boilerplate for applying mixins to MatExpansionPanel.
Expand Down Expand Up @@ -79,8 +80,16 @@ export const EXPANSION_PANEL_ANIMATION_TIMING = '225ms cubic-bezier(0.4,0.0,0.2,
})
export class MatExpansionPanel extends _MatExpansionPanelMixinBase
implements CanDisable, OnChanges, OnDestroy {

/** Whether the toggle indicator should be hidden. */
@Input() hideToggle: boolean = false;
@Input()
get hideToggle(): any {
Copy link
Member

Choose a reason for hiding this comment

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

any -> boolean

return this._hideToggle;
}
set hideToggle(value: any) {
this._hideToggle = coerceBooleanProperty(value);
}
private _hideToggle = false;

/** Stream that emits for changes in `@Input` properties. */
_inputChanges = new Subject<SimpleChanges>();
Expand Down
10 changes: 9 additions & 1 deletion src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {throwMatMenuInvalidPositionX, throwMatMenuInvalidPositionY} from './menu
import {MatMenuItem} from './menu-item';
import {MatMenuPanel} from './menu-panel';
import {MenuPositionX, MenuPositionY} from './menu-positions';
import {coerceBooleanProperty} from '@angular/cdk/coercion';


/** Default `mat-menu` options that can be overridden. */
Expand Down Expand Up @@ -120,7 +121,14 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {
@ContentChildren(MatMenuItem) items: QueryList<MatMenuItem>;

/** Whether the menu should overlap its trigger. */
@Input() overlapTrigger = this._defaultOptions.overlapTrigger;
@Input()
set overlapTrigger(value: boolean) {
this._overlapTrigger = coerceBooleanProperty(value);
}
get overlapTrigger(): boolean {
return this._overlapTrigger;
}
private _overlapTrigger: boolean = this._defaultOptions.overlapTrigger;

/**
* This method takes classes set on the host mat-menu element and applies them on the
Expand Down
2 changes: 1 addition & 1 deletion src/lib/slide-toggle/slide-toggle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export class MatSlideToggle extends _MatSlideToggleMixinBase implements OnDestro
@Input()
get checked(): boolean { return this._checked; }
set checked(value) {
this._checked = !!value;
this._checked = coerceBooleanProperty(value);
this._changeDetectorRef.markForCheck();
}
/** An event will be dispatched each time the slide-toggle changes its value. */
Expand Down
4 changes: 2 additions & 2 deletions src/lib/tabs/tab-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
ViewChild,
ViewEncapsulation,
} from '@angular/core';
import {coerceBooleanProperty} from '@angular/cdk/coercion';
import {coerceBooleanProperty, coerceNumberProperty} from '@angular/cdk/coercion';
import {Subscription} from 'rxjs/Subscription';
import {MatTab} from './tab';
import {merge} from 'rxjs/observable/merge';
Expand Down Expand Up @@ -111,7 +111,7 @@ export class MatTabGroup extends _MatTabGroupMixinBase implements AfterContentIn

/** The index of the active tab. */
@Input()
set selectedIndex(value: number | null) { this._indexToSelect = value; }
set selectedIndex(value: number | null) { this._indexToSelect = coerceNumberProperty(value); }
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the author was intentionally trying to allow null here in which case I think you want coerceNumberProperty(value, null)

Copy link
Contributor Author

@amcdnl amcdnl Oct 1, 2017

Choose a reason for hiding this comment

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

The coerceNumberProperty signature does not support null as the second parameter. Should we extend it to support that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, in a lot of cases people really do just want number. What if we did this?

export function coerceNumberProperty(value: any): number;
export function coerceNumberProperty<D>(value: any, fallback: D): number | D;
export function coerceNumberProperty(value: any, fallbackValue = 0) { ... }

It would allow people to get just a number in 1-arg case and number|whatever in 2-arg case. @jelbourn WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

That seems good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works well.

get selectedIndex(): number | null { return this._selectedIndex; }
private _selectedIndex: number | null = null;

Expand Down
2 changes: 2 additions & 0 deletions src/lib/tabs/tab-header.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {fromEvent} from 'rxjs/observable/fromEvent';
import {merge} from 'rxjs/observable/merge';
import {of as observableOf} from 'rxjs/observable/of';
import {Subscription} from 'rxjs/Subscription';
import {coerceNumberProperty} from '@angular/cdk/coercion';
import {MatInkBar} from './ink-bar';
import {MatTabLabelWrapper} from './tab-label-wrapper';

Expand Down Expand Up @@ -120,6 +121,7 @@ export class MatTabHeader extends _MatTabHeaderMixinBase
@Input()
get selectedIndex(): number { return this._selectedIndex; }
set selectedIndex(value: number) {
value = coerceNumberProperty(value);
this._selectedIndexChanged = this._selectedIndex != value;
this._selectedIndex = value;
this._focusIndex = value;
Expand Down