feat(datepicker): add keyboard support to calendar #3655

mmalerba · 2017-03-17T16:12:07Z

temporary demo: https://mmalerba-demo1.firebaseapp.com/datepicker

exact active date

kara

Mostly nits. I did play with the demo, and I was wondering about the focus of the datepicker overlay. When I open the overlay with the keyboard, the focus doesn't seem to be on the overlay, so I have to hit TAB a few more times to get to the calendar body. At first, it doesn't seem like it's keyboard navigable for this reason. Can we explicitly focus the panel when it opens to make this more obvious?

kara · 2017-03-23T03:29:12Z

src/lib/datepicker/calendar.ts

+  /**
+   * Adds the given number of months to the date. Months are counted as if flipping 12 pages for
+   * each year on a calendar and then finding the closest date in the new month. For example when
+   * adding 1 year to Feb 29 2016 the resulting date will be Feb 28 2017.


Ultra nit: add comma after 'Feb 29, 2016' (same for line 241)? Love the detailed comment though

kara · 2017-03-23T03:31:09Z

src/lib/datepicker/calendar.ts

+   * Jan 31 2017 the resulting date will be Feb 28 2017.
+   */
+  private _addCalendarMonths(date: SimpleDate, months: number): SimpleDate {
+    let newDate = date.add({'months': months});


You can shorten this to

let newDate = date.add({months})

(same on line 235)

kara · 2017-03-23T03:43:31Z

src/lib/datepicker/calendar.ts

    return this._monthView ?
        date1.year == date2.year && date1.month == date2.month :
        date1.year == date2.year;
  }
+
+  /** Handles keydown events on the calendar body. */
+  _handleCalendarBodyKeydown(event: KeyboardEvent): void {


Considered breaking this logic out into its own mini-service?

Also, have you thought about separating into two functions -one for month view and one for year view? Not sure if that will actually shorten anything, but it may be easier to scan it without the conditionals.

I'm not sure what we'd gain by making it a service, I split out 2 separate functions for month view and year view though

Just thought the file was getting pretty long, but if you don't think so, doesn't matter too much.

kara · 2017-03-23T03:47:27Z

src/lib/datepicker/calendar.spec.ts

+
+      it('should not allow selection of disabled date in month view', () => {
+        expect(calendarInstance._monthView).toBe(true);
+        expect(calendarInstance._activeDate).toEqual(new SimpleDate(2017, 0, 1));


Curious about this behavior. It seems like we allow people to focus the disabled dates, but not select them, whereas AngularJS Material doesn't allow highlighting of disabled dates at all. What's the a11y best practice for this?

Yeah this was more of a practical decision, since someone could theoretically create a calendar with no maxDate but a date filter that disables all dates > 1/1/17. In this case if the user tried to arrow past 1/1/17 everything would just lock up as it searched for the next enabled date. I put a TODO and I'm curious to see how AngularJS Material handled this

kara · 2017-03-23T03:51:15Z

src/lib/datepicker/calendar.spec.ts

+    });
+  });
+
+  describe('a11y', () => {


Nit: Two describe blocks both named a11y?

One of them is MdCalendar.calendar with date filter.a11y the other is MdCalendar.a11y (though it should be MdCalendar.standard calendar.a11y so making that change).

Aha I see. The diff was a bit misleading.

mmalerba · 2017-03-23T19:00:32Z

Yeah focus state when you open the calendar is still kind of wonky, I'll address that in a different PR. Addressed other comments, PTAL.

kara · 2017-03-24T22:19:29Z

src/lib/datepicker/calendar.ts

+  /**
+   * Adds the given number of months to the date. Months are counted as if flipping pages on a
+   * calendar and then finding the closest date in the new month. For example when adding 1 month to
+   * Jan 31, 2017 the resulting date will be Feb 28, 2017.


Haha, sorry for being unclear. I meant a comma after "2017", not in the date.

For example, when adding 1 month to Jan 31, 2017, the resulting date will be ...

kara

LGTM. Apply merge label when ready.

* add active cell support to calendar-table, month-view, and year-view * stop normalizing the active date in calendar since we now care about the exact active date * add some key handlers * add keyboard support, break some tests * add tests * some finishing touches * fix tabindex * fix rxjs import * addressed some comments * refactor handleKeydown * fix commas

angular-automatic-lock-bot · 2019-09-08T16:01:39Z

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.}

mmalerba added the pr: needs review label Mar 17, 2017

mmalerba assigned jelbourn and kara Mar 17, 2017

mmalerba requested review from jelbourn and kara March 17, 2017 16:12

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 17, 2017

mmalerba force-pushed the datepicker branch from f43df39 to 630c8e5 Compare March 17, 2017 21:53

mmalerba force-pushed the dp branch from cf660fb to b86d03c Compare March 17, 2017 22:42

mmalerba added 8 commits March 20, 2017 12:10

add active cell support to calendar-table, month-view, and year-view

6b3e8ec

stop normalizing the active date in calendar since we now care about the

ffda7d3

exact active date

add some key handlers

2b93c67

add keyboard support, break some tests

fdee139

add tests

6a59fc0

some finishing touches

eaa34f6

fix tabindex

d684d6f

fix rxjs import

356bac8

mmalerba force-pushed the dp branch from b86d03c to 356bac8 Compare March 20, 2017 19:10

kara reviewed Mar 23, 2017

View reviewed changes

kara unassigned jelbourn and kara Mar 23, 2017

mmalerba added 2 commits March 23, 2017 10:35

addressed some comments

948335e

refactor handleKeydown

c7f3cfc

mmalerba assigned kara Mar 23, 2017

kara reviewed Mar 24, 2017

View reviewed changes

kara approved these changes Mar 24, 2017

View reviewed changes

kara added pr: lgtm and removed pr: needs review labels Mar 24, 2017

fix commas

0ee27c3

mmalerba merged commit 5170bed into angular:datepicker Mar 24, 2017

mmalerba deleted the dp branch April 3, 2018 15:16

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): add keyboard support to calendar #3655

feat(datepicker): add keyboard support to calendar #3655

mmalerba commented Mar 17, 2017 •

edited

Loading

kara left a comment •

edited

Loading

kara Mar 23, 2017

kara Mar 23, 2017

kara Mar 23, 2017

mmalerba Mar 23, 2017

kara Mar 24, 2017

kara Mar 23, 2017

mmalerba Mar 23, 2017

kara Mar 23, 2017

mmalerba Mar 23, 2017

kara Mar 23, 2017

mmalerba commented Mar 23, 2017

kara Mar 24, 2017

kara left a comment

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

feat(datepicker): add keyboard support to calendar #3655

feat(datepicker): add keyboard support to calendar #3655

Conversation

mmalerba commented Mar 17, 2017 • edited Loading

kara left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Mar 23, 2017

Choose a reason for hiding this comment

kara left a comment

Choose a reason for hiding this comment

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

mmalerba commented Mar 17, 2017 •

edited

Loading

kara left a comment •

edited

Loading