refactor(datepicker): move keyboard events handlers into the views by julianobrasil · Pull Request #9775 · angular/components

julianobrasil · 2018-02-04T13:26:12Z

~~This should be blocked until #9727 is merged.~~

~~@mmalerba, as #9727 hasn't been merged yet, I can merge this into that one.~~

There are a few things to discuss before though. Originally the keyboard events (basically (keydown)) were listened to at this markup:

https://github.com/angular/material2/blob/c7201984d4570e2bf17fbf4afce0dc4400f863f5/src/lib/datepicker/calendar.html#L23-L24

As now the events are being handled inside each view, I moved the listening part to each view markup, like the snippet below, from month-view.html

<tableclass="mat-calendar-table"><theadclass="mat-calendar-table-header"><tr><th*ngFor="let day of _weekdays" [attr.aria-label]="day.long">{{day.narrow}}</th></tr><tr><thclass="mat-calendar-table-header-divider" colspan="7" aria-hidden="true"></th></tr></thead><tbodymat-calendar-body[label]="_monthLabel"
[rows]="_weeks"
[todayValue]="_todayDate"
[selectedValue]="_selectedDate"
[labelMinRequiredCells]="3"
[activeCell]="_dateAdapter.getDate(activeDate) - 1"
(selectedValueChange)="_dateSelected($event)"
(keydown)="_handleCalendarBodyKeydown($event)"><-------HEREISTHELISTENERNOW</tbody></table>

Also, I thought it'd be better to move the a11y tests from calendar.spec.ts to each view spec file.

As a consequence... well, I'm with this feeling that we're replicating too much code (just a feeling).

Another possible approach would be to keep the listener in calendar.html (its original place) and pass through keydown event to each view, but I thought it'd be a little bit awkward.

mmalerba

This feels much better to me, having logic for each of the views mixed into the calendar definitely felt wrong

mmalerba · 2018-02-15T16:54:32Z

- this._getValidDateOrNull(this._dateAdapter.deserialize(value)) || this._dateAdapter.today();
+ const oldActiveDate = this._activeDate;
+ const validDate =
+ this._getValidDateOrNull(this._dateAdapter.deserialize(value)) || this._dateAdapter.today();


nit: indent continuation lines by 4

Oh, I didn't know that was the rule. 😃

mmalerba · 2018-02-15T16:59:33Z

 }

+ /** Focuses the active cell after the microtask queue is empty. */
+ _focusActiveCell() {


could we move this logic to the calendar table instead of repeating it?

mmalerba · 2018-02-15T17:06:57Z

+ this._userSelected();
+ }
+
+ _userSelected(): void {


nit: I don't think this needs to be its own function, this._userSelected() is only a couple characters shorter than this._userSelection.emit()

mmalerba

needs rebase

mmalerba · 2018-02-15T21:51:35Z

 @Output() readonly selectedValueChange: EventEmitter<number> = new EventEmitter<number>();

+ constructor(private _elementRef: ElementRef,
+ private _ngZone: NgZone) {


looks like this will fit on one line?

mmalerba · 2018-02-15T21:55:53Z

 this._init();
 }
+ if (this._dateAdapter.compareDate(oldActiveDate, this._activeDate)) {
+ this.activeDateChange.emit(this._activeDate);


are we sure we want to emit here? it seems strange to emit in response to the @Input() changing. I would have expected it to only emit if the active date changed due to user interaction (click, keyboard, etc)

It's here for convenience (just because there was a single line above saving the previous activeDate 😸), but you're right. This is far from being the best place to put it. I'll move it.

…9775)

Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can *way* better report invalid signatures for constructor changes like for the `MatCalendar` (angular#9775). Just relying on the length of arguments means that *order* is being ignored. This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).

…12970) Instead of just checking the length of the constructor arguments, we now check the types of the constructor or super call. This means that we can *way* better report invalid signatures for constructor changes like for the `MatCalendar` (#9775). Just relying on the length of arguments means that *order* is being ignored. This also makes maintaining the constructor signature changes easier (there are a lot of instances for V7).

angular-automatic-lock-bot · 2019-09-08T14:01:33Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

julianobrasil requested a review from mmalerba as a code owner February 4, 2018 13:26

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 4, 2018

julianobrasil changed the title ~~refac(datepicker): move keyboard hanlders into the views~~refacor(datepicker): move keyboard hanlders into the viewsFeb 4, 2018

julianobrasil requested a review from jelbourn as a code owner February 4, 2018 18:13

julianobrasil changed the title ~~refacor(datepicker): move keyboard hanlders into the views~~refactor(datepicker): move keyboard hanlders into the viewsFeb 4, 2018

julianobrasil changed the title ~~refactor(datepicker): move keyboard hanlders into the views~~refactor(datepicker): move keyboard events handlers into the viewsFeb 4, 2018

mmalerba reviewed Feb 15, 2018
View reviewed changes

mmalerba added the target: major This PR is targeted for the next major release label Feb 15, 2018

julianobrasil added 5 commits February 15, 2018 18:27

add max/min filter to multi year and year views
5eb3b3a

address@mmalerba's comments
9bcaeaa

fix and tighten up logic
4bed1b1

Move datepicker handlers to view components
4572dd2

move set focus function to MatCalendarBody
7f1bbd2

mmalerba reviewed Feb 15, 2018
View reviewed changes

rebasing and commnets...
8bdf285

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Feb 16, 2018

mmalerba approved these changes Feb 16, 2018
View reviewed changes

jelbourn merged commit 8e3ab8d into angular:masterFeb 18, 2018

tinayuangao pushed a commit that referenced this pull request Feb 20, 2018
refactor(datepicker): move keyboard events handlers into the views (#…
916d3ba
…9775)

julianobrasil deleted the refacDatePicker2 branch March 30, 2018 11:07

devversion mentioned this pull request Sep 4, 2018
refactor(ng-update): better check for updated constructor signatures #12970
Merged

angular-automatic-lock-botBot locked and limited conversation to collaborators Sep 8, 2019

Navigation Menu

Uh oh!

refactor(datepicker): move keyboard events handlers into the views - #9775

refactor(datepicker): move keyboard events handlers into the views#9775
jelbourn merged 6 commits into
angular:masterfrom
julianobrasil:refacDatePicker2

julianobrasil commented Feb 4, 2018•
edited
Loading

Uh oh!

mmalerba left a comment

Uh oh!

mmalerbaFeb 15, 2018

Uh oh!

julianobrasilFeb 15, 2018

Uh oh!

mmalerbaFeb 15, 2018

Uh oh!

julianobrasilFeb 15, 2018

Uh oh!

mmalerbaFeb 15, 2018

Uh oh!

mmalerba left a comment

Uh oh!

mmalerbaFeb 15, 2018

Uh oh!

mmalerbaFeb 15, 2018

Uh oh!

julianobrasilFeb 16, 2018•
edited
Loading

Uh oh!

angular-automatic-lock-botBot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

julianobrasil commented Feb 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerbaFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

julianobrasilFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerbaFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

julianobrasilFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerbaFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerba left a comment

Choose a reason for hiding this comment

Uh oh!

mmalerbaFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

mmalerbaFeb 15, 2018

Choose a reason for hiding this comment

Uh oh!

julianobrasilFeb 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-botBot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

julianobrasil commented Feb 4, 2018•
edited
Loading

julianobrasilFeb 16, 2018•
edited
Loading