feat(datepicker): add keyboard navigation #1551

maxokorokov · 2017-05-15T14:05:40Z

This essentially contains two things:

All the work @divdavem has done for adding keyboard navigation (feat(datepicker): add keyboard navigation #1273)
Complete refactoring or the datepicker internals

Problems with the datepicker before refactoring:

All the logic is inside of the NgbDatepicker component - became a huge mess after adding of the keyboard handling
Very little unit tests for the logic because of the above

Refactoring:

Public API unchanged
NgbDatepicker component is as dumb as possible, just displays DatepickerViewModel and propagates @Inputs to the service
NgbDatepickerService generates the DatepickerViewModel with a very simple API
NgbDatepickerKeyMappingService handles key event mapping to actions
Many more unit tests for the service
OnPush everywhere for the datepicker components

So now it looks something like this:

P.S. It might seem a lot, but 2/3 of the PR are unit tests
P.P.S. Tried lots of different solutions based on observables, but for now they add only boilerplate code...

divdavem

It is a great PR. Thank you for your hard work! I have added some remarks. Note that I have not reviewed test files yet.

divdavem · 2017-06-09T09:17:23Z

src/datepicker/datepicker-day-view.ts

@@ -20,14 +21,16 @@ import {NgbDateStruct} from './ngb-date-struct';
    '[class.text-white]': 'selected',
    '[class.text-muted]': 'isMuted()',
    '[class.outside]': 'isMuted()',
-    '[class.btn-secondary]': '!disabled'
+    '[class.btn-secondary]': '!disabled',


As discussed, the btn-secondary class should be there without the !disabled condition, so that it is possible to see the focus even on disabled dates.

divdavem · 2017-06-09T09:50:30Z

src/datepicker/datepicker-service.ts

+    // rebuilding months
+    if (startDate) {
+      const forceRebuild =
+          !!patch['firstDayOfWeek'] || !!patch['markDisabled'] || !!patch['minDate'] || !!patch['maxDate'];


As discussed, !!patch['firstDayOfWeek'] does not exactly mean that patch contains firstDayOfWeek, but that it contains it and it is neither 0 nor any other falsy value... firstDayOfWeek could be 0 in some calendars (even if 0 is not used in the default calendar), and in that case, we would still need to have forceRebuild = true.

I think it would be better to use instead the 'firstDayOfWeek' in patch syntax.
The same should be done everywhere in this function ('minDate' in patch, forceRebuild = 'firstDayOfWeek' in patch || 'markDisabled' in patch || 'minDate' in patch || 'maxDate' in patch ...) so that we do not have to wonder whether each value can be falsy.

divdavem · 2017-06-09T10:01:00Z

src/datepicker/datepicker.ts

+    });
+
+    const modelSubscription = _service.model$.subscribe(model => {
+      const newDate = model.months[0].firstDate;


This could be written more simply: newDate = model.firstDate
and the same for oldDate.

divdavem · 2017-06-09T10:02:54Z

src/datepicker/datepicker.ts

-        [minDate]="_minDate"
-        [maxDate]="_maxDate"
-        [months]="months.length"
+        [date]="model.months[0].firstDate"


This could be written more simply model.firstDate.

divdavem · 2017-06-09T10:03:42Z

src/datepicker/datepicker.ts


  onNavigateEvent(event: NavigationEvent) {
    switch (event) {
      case NavigationEvent.PREV:
-        this._setViewWithinLimits(this._calendar.getPrev(this.months[0].firstDate, 'm'));
+        this._service.open(this._calendar.getPrev(this.model.months[0].firstDate, 'm', 1));


This could be written more simply this.model.firstDate (and the same for line 316).

divdavem · 2017-06-09T10:07:58Z

src/datepicker/datepicker-service.ts

+      state.months = months;
+      state.firstDate = months.length > 0 ? months[0].firstDate : undefined;
+      state.lastDate = months.length > 0 ? months[months.length - 1].lastDate : undefined;
+      state.viewDate = state.firstDate;


As viewDate seems to always be the same as firstDate, is it really needed to have both? Maybe we could remove viewDate from the model and only keep firstDate.

maxokorokov · 2017-06-12T12:44:34Z

Thanks, @divdavem!

Amended fixes for all your comments.
Also added the BREAKING CHANGE label because of the OnPush introduction

maxokorokov · 2017-06-29T10:36:08Z

Aligned with master, resolved conflicts

pkozlowski-opensource · 2017-06-29T12:43:06Z

@maxokorokov I don't see TravisCI being run on those commits, I'm not sure it is because of getting commits from multiple people. But I want to see "green tick" before merging this one :-)

pkozlowski-opensource · 2017-06-29T12:46:09Z

src/datepicker/datepicker-navigation-select.ts

+      class="custom-select d-inline-block"
+      [value]="date?.month"
+      (change)="changeMonth($event.target.value)"
+      tabindex="-1">


Why tabindex="-1"?

In the current implementation its the ngb-datepicker itself that captures all events and prevents default behaviour to avoid page scrolling when hitting arrows, space, etc.

Now that I think of, it might be better in future to focus only a specific date instead of everything, so the default navigation behaviour with keyboard select boxes and arrows would continue working as before

pkozlowski-opensource · 2017-06-29T12:50:38Z

src/datepicker/datepicker.spec.ts

@@ -541,6 +572,143 @@ describe('ngb-datepicker', () => {
      expect(getYearSelect(fixture.nativeElement).value).toBe(`${today.getFullYear()}`);
    });

+    it('should correctly navigate with keyboard', () => {


This test is quite big, and if one condition fails we would have hard time figuring out what failed exactly.

Could you split it into multiple small tests? (ideally one test for one navigation key)?

Think of it of as a big integration test :)
key-mapping service has unit tests of its own, but yes, we should split this one

pkozlowski-opensource · 2017-06-29T13:05:02Z

Great stuff @divdavem @maxokorokov ! I've left some comments. The diff is big so I can't really comment on every line of code, but I haven't seen anything scary :-)

@maxokorokov could you please expand on the "BREAKING CHANGES" (that is, take it from the user's point of view?)

maxokorokov · 2017-07-10T09:42:59Z

Split the datepicker.spec.ts keyboard test into smaller pieces

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

BREAKING CHANGE: component uses `ChangeDetectionStrategy.OnPush` now for most of the internal implementation. Things like the dynamic internationalization or calendar change might not work anymore as these are injected services. Any internal changes in these services in runtime will not trigger datepicker re-rendering.

maxokorokov added component: datepicker needs: review type: accessibility labels May 15, 2017

maxokorokov mentioned this pull request May 15, 2017

feat(datepicker): add keyboard navigation #1273

Closed

divdavem reviewed Jun 9, 2017

View reviewed changes

maxokorokov force-pushed the dp_refactoring branch from af09af8 to 18ee793 Compare June 12, 2017 12:41

maxokorokov force-pushed the dp_refactoring branch from 18ee793 to 5a12fdb Compare June 29, 2017 10:19

maxokorokov assigned pkozlowski-opensource Jun 29, 2017

maxokorokov force-pushed the dp_refactoring branch from 5a12fdb to f6b7e02 Compare June 29, 2017 12:36

pkozlowski-opensource reviewed Jun 29, 2017

View reviewed changes

pkozlowski-opensource added this to the 1.0.0-alpha.28 milestone Jun 30, 2017

This was referenced Jul 3, 2017

Datepicker not clickable when used at the same page with Highcharts #1654

Closed

datepicker HostListener mouse issue v27 #1660

Closed

Datepicker bug in firefox browser #1615

Closed

maxokorokov force-pushed the dp_refactoring branch from f6b7e02 to 3e04658 Compare July 10, 2017 09:40

maxokorokov added needs: merging and removed needs: review labels Jul 10, 2017

divdavem and others added 2 commits July 10, 2017 16:20

maxokorokov force-pushed the dp_refactoring branch from 3e04658 to 0a2c555 Compare July 10, 2017 14:20

pkozlowski-opensource closed this in fc8a2ae Jul 10, 2017

maxokorokov mentioned this pull request Jul 10, 2017

Datepicker evolution #862

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): add keyboard navigation #1551

feat(datepicker): add keyboard navigation #1551

maxokorokov commented May 15, 2017

divdavem left a comment

divdavem Jun 9, 2017

divdavem Jun 9, 2017

divdavem Jun 9, 2017

divdavem Jun 9, 2017

divdavem Jun 9, 2017

divdavem Jun 9, 2017

maxokorokov commented Jun 12, 2017

maxokorokov commented Jun 29, 2017

pkozlowski-opensource commented Jun 29, 2017

pkozlowski-opensource Jun 29, 2017

maxokorokov Jun 29, 2017

pkozlowski-opensource Jun 29, 2017

maxokorokov Jun 29, 2017

pkozlowski-opensource commented Jun 29, 2017

maxokorokov commented Jul 10, 2017 •

edited

feat(datepicker): add keyboard navigation #1551

feat(datepicker): add keyboard navigation #1551

Conversation

maxokorokov commented May 15, 2017

divdavem 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

maxokorokov commented Jun 12, 2017

maxokorokov commented Jun 29, 2017

pkozlowski-opensource commented Jun 29, 2017

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

pkozlowski-opensource commented Jun 29, 2017

maxokorokov commented Jul 10, 2017 • edited

maxokorokov commented Jul 10, 2017 •

edited