fix(datepicker): better support for input and change events #4826
Conversation
googlebot
added
the
cla: yes
src/lib/datepicker/datepicker.md
Outdated
|
|
||
| ### Input and change events | ||
| The input's native `input` and `change` events will only trigger due to user interaction with the | ||
| input element, they will not fire when the user selects a date from the calendar popup. Because of |
There was a problem hiding this comment.
Grammar nit: with the input element, they will not fire => with the input element; they will not fire
src/lib/datepicker/datepicker.md
Outdated
| The input's native `input` and `change` events will only trigger due to user interaction with the | ||
| input element, they will not fire when the user selects a date from the calendar popup. Because of | ||
| this limitation, the datepicker input also has support for `dateInput` and `dateChange` events, | ||
| these trigger when the user interacts with either the input or the popup. |
There was a problem hiding this comment.
Grammar nit: events, these trigger => events. These trigger when
| @@ -43,6 +44,21 @@ export const MD_DATEPICKER_VALIDATORS: any = { | |||
| }; | |||
|
|
|||
|
|
|||
| /** An event used for datepicker input and change events. */ | |||
There was a problem hiding this comment.
Nit: Maybe add something to the comment about why you don't just use the regular event objects from (input) and (change).
| @@ -152,7 +174,7 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces | |||
| Validators.compose([this._minValidator, this._maxValidator, this._filterValidator]); | |||
|
|
|||
| constructor( | |||
| private _elementRef: ElementRef, | |||
| public _elementRef: ElementRef, | |||
There was a problem hiding this comment.
Have you considered passing the element and the value to MdDatepickerInputEvent separately (instead of this), so you can keep _elementRef private?
| testComponent = fixture.componentInstance; | ||
| inputEl = fixture.debugElement.query(By.css('input')).nativeElement; | ||
|
|
||
| spyOn(testComponent, 'onChange'); |
There was a problem hiding this comment.
Another option would be to use jasmine.createSpy() here. It would remove the need for creating the empty functions in lines 720 on.
testComponent.onChange = jasmine.createSpy('onChange');
testComponent.onInput = jasmine.createSpy('onInput');
...There was a problem hiding this comment.
It won't let me just add properties like that, it complains that onChange isn't a member of testComponent
There was a problem hiding this comment.
Oh, I guess you'd need to add them on the testComponent too anyway. So I guess it doesn't really help.
|
@kara whoops forgot about this for a while, but comments addressed, PTAL |
|
rebased |
| /** | ||
| * An event used for datepicker input and change events. We don't always have access to a native | ||
| * input or change event because the event may have been triggered by the user clicking on the | ||
| * calendar popup. For consistency we always use MdDatepickerInputEvent instead. |
There was a problem hiding this comment.
Nit: For consistency we always -> For consistency, we always use
| @@ -134,6 +152,10 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces | |||
| } | |||
| private _disabled: boolean; | |||
|
|
|||
| @Output() dateChange = new EventEmitter<MdDatepickerInputEvent<D>>(); | |||
|
|
|||
| @Output() dateInput = new EventEmitter<MdDatepickerInputEvent<D>>(); | |||
There was a problem hiding this comment.
Add user-facing JsDoc for these events
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
fixes: #4615