feat(datepicker): add keyboard navigation #1273

divdavem · 2017-01-30T18:11:27Z

The following keyboard shortcuts are now available:
Arrow left/right: previous/next day
Arrow up/down: previous/next week
Page up/down: previous/next month
Shift+page up/down: previous/next year
Home/end: beginning/end of the currently displayed month(s)
Shift+home/end: min/max selectable date

Note that the previous/next month and previous/next year shortcuts should probably keep the current day in the previous/next month and the current month in the previous/next year. I think this should be implemented by a change in the behavior of getNext/getPrev in ngb-calendar. This probably has to be discussed, especially with @maxokorokov who designed this API. I have opened another PR about it: #1277

maxokorokov

Looks good, thanks!

A couple of comments I have:

The only big concern I have is that now keyboard focusing and navigation between months are very much tied to each other in the code.

I would have done focus handling as an optional service that works with the datepicker. Now it starts to get huge, so I think we should think about something like that. Currently 25% of the datapicker.ts code is focus handling

I'm not sure everybody would want to have the keyboard support ON all the time. Like for time range selection cases, etc. But this could be optionally turned off later I guess

Minor comments:

when two months are displayed, clicking on navigation arrows changes focus between months, when it should do navigation actually
please order members/methods alphabetically

maxokorokov · 2017-02-14T16:51:13Z

src/datepicker/datepicker.spec.ts

+  return event;
+}
+
+function triggerFocus(element: DebugElement) {


unused function I suppose

maxokorokov · 2017-02-14T16:54:00Z

src/datepicker/datepicker.ts

@@ -250,17 +260,26 @@ export class NgbDatepicker implements OnChanges,
    }
  }

-  onDateSelect(date: NgbDate) {
-    this._setViewWithinLimits(date);
+  isDisplayedDateSelectable(date: NgbDate) {


can be private _isDisplayedDateSelectable

maxokorokov · 2017-02-14T16:57:22Z

src/datepicker/datepicker.ts

        break;
    }

    this._updateData();
  }

+  @HostListener('keydown', ['$event'])


In other components the events are usually grouped in the host like so:

(keydown): onKeyDown($event)

I think I would prefer to do the same here to quickly see what kind of handlers we have on the datepicker

maxokorokov · 2017-02-14T17:00:17Z

src/datepicker/datepicker.ts

+      return;
+    }
+    switch (event.keyCode) {
+      case 33 /* page up */:


In rating and typeahead we have an enum Keys that contains key codes (and ideally should be shared), so switch reads like: case Key.PageUp:

Not sure which one I prefer though...

maxokorokov · 2017-02-14T17:01:20Z

src/datepicker/datepicker.ts

+  }
+
+  @HostListener('blur', ['$event'])
+  onBlur(event: FocusEvent) {


could be '(blur)': 'focusedDate = null' I suppose

maxokorokov · 2017-02-14T17:02:49Z

src/datepicker/datepicker.ts

        break;
    }

    this._updateData();
  }

+  @HostListener('keydown', ['$event'])
+  onKeyDown(event: KeyboardEvent) {
+    let focusedDate = this.focusedDate;


could be const, or just use this.focusedDate altogether?

maxokorokov · 2017-02-14T17:08:47Z

src/datepicker/datepicker.ts

@@ -310,6 +416,46 @@ export class NgbDatepicker implements OnChanges,
    }
  }

+  private getFirstDisplayedDate() { return this.months[0].firstDate; }


method name should start with _

maxokorokov · 2017-02-14T17:08:59Z

src/datepicker/datepicker.ts

@@ -310,6 +416,46 @@ export class NgbDatepicker implements OnChanges,
    }
  }

+  private getFirstDisplayedDate() { return this.months[0].firstDate; }
+
+  private getLastDisplayedDate() {


method name should start with _

divdavem · 2017-02-16T14:52:20Z

@maxokorokov Thank you for your review. I have pushed some changes following your comments.

Regarding the idea of implementing keyboard navigation as a separate service, I had actually asked the question to @pkozlowski-opensource but he had answered to keep it simple in the same file for now.
We can do it later for sure to allow customization of navigation shortcuts, but, however, I don't see the point of actually disabling keyboard navigation. Cases such as range-selection can also benefit from keyboard navigation.

divdavem · 2017-03-29T13:55:04Z

@maxokorokov I have rebased this PR and resolved the conflict with the master branch. Do you think it could be merged soon?

maxokorokov · 2017-04-13T16:20:31Z

As we've discussed @divdavem, we'll try to refactor this quite a bit:

merge navigation and focusing logic, because it's essentially the same thing
isolate it into something like NgbDatepickerNavigationService and remove from NgbDatepicker itself

Refactoring required

The following shortcuts are available: Arrow left/right: previous/next day Arrow up/down: previous/next week Page up/down: previous/next month Shift+page up/down: previous/next year Home/end: beginning/end of the current view Shift+home/end: min/max selectable date

maxokorokov · 2017-05-15T14:07:38Z

Moved everything from from here in the #1551, closing this one.

divdavem force-pushed the datepickerKeyboardNavigation branch 7 times, most recently from 2ad3d45 to 76f5994 Compare January 31, 2017 15:59

divdavem mentioned this pull request Jan 31, 2017

feat(datepicker): keep day/month when adding/removing month/year in ngb-calendar #1277

Closed

divdavem force-pushed the datepickerKeyboardNavigation branch 2 times, most recently from ea6bee6 to efcae51 Compare February 1, 2017 14:39

divdavem changed the title ~~feat(datepicker): add keyboard navigation (work in progress)~~ feat(datepicker): add keyboard navigation Feb 1, 2017

divdavem force-pushed the datepickerKeyboardNavigation branch from efcae51 to 6ccfa4d Compare February 1, 2017 15:49

divdavem mentioned this pull request Feb 1, 2017

feat(datepicker): add accessibility (work in progress) #1281

Closed

2 tasks

maxokorokov added component: datepicker needs: review labels Feb 14, 2017

maxokorokov previously requested changes Feb 14, 2017

View reviewed changes

divdavem force-pushed the datepickerKeyboardNavigation branch 2 times, most recently from 8ad8679 to f03190d Compare February 16, 2017 14:48

divdavem force-pushed the datepickerKeyboardNavigation branch from f03190d to 97e6d91 Compare March 15, 2017 14:09

maxokorokov mentioned this pull request Mar 29, 2017

Datepicker evolution #862

Closed

10 tasks

divdavem force-pushed the datepickerKeyboardNavigation branch 2 times, most recently from cd1e3f7 to fd7d0b3 Compare March 29, 2017 13:50

pkozlowski-opensource added the type: accessibility label Apr 11, 2017

maxokorokov added needs: design and removed needs: review labels Apr 13, 2017

maxokorokov added the needs: work label Apr 13, 2017

divdavem added 4 commits April 18, 2017 14:07

Code review: minor changes

5b4fb84

Code review: re-ordering methods / fields alphabetically

7fc55de

Code review: fixing navigation arrows when multiple months are displayed

94bbea1

divdavem force-pushed the datepickerKeyboardNavigation branch from fd7d0b3 to 94bbea1 Compare April 18, 2017 12:07

maxokorokov mentioned this pull request May 15, 2017

feat(datepicker): add keyboard navigation #1551

Closed

maxokorokov closed this May 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): add keyboard navigation #1273

feat(datepicker): add keyboard navigation #1273

divdavem commented Jan 30, 2017 •

edited

maxokorokov left a comment

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

maxokorokov Feb 14, 2017

divdavem commented Feb 16, 2017

divdavem commented Mar 29, 2017

maxokorokov commented Apr 13, 2017

maxokorokov commented May 15, 2017

feat(datepicker): add keyboard navigation #1273

feat(datepicker): add keyboard navigation #1273

Conversation

divdavem commented Jan 30, 2017 • edited

maxokorokov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divdavem commented Feb 16, 2017

divdavem commented Mar 29, 2017

maxokorokov commented Apr 13, 2017

maxokorokov commented May 15, 2017

divdavem commented Jan 30, 2017 •

edited