fix(datepicker): add max/min filter to multi year and year views #9727
Conversation
googlebot
added
the
cla: yes
src/lib/datepicker/calendar.html
Outdated
| @@ -36,6 +36,8 @@ | |||
| [activeDate]="_activeDate" | |||
| [selected]="selected" | |||
| [dateFilter]="_dateFilterForViews" | |||
There was a problem hiding this comment.
I think you can remove _dateFilterForViews now and just use the date filter that was passed to the calendar
src/lib/datepicker/year-view.ts
Outdated
| this._dateAdapter.getYear(this.activeDate), month, 1); | ||
| const activeYear = this._dateAdapter.getYear(this.activeDate); | ||
|
|
||
| let firstOfMonth = this._dateAdapter.createDate(activeYear, month, 1); |
src/lib/datepicker/year-view.ts
Outdated
| return true; | ||
| } | ||
|
|
||
| _yearAndMonthAfterMin(year: number, month: number) { |
src/lib/datepicker/calendar.ts
Outdated
| @@ -346,7 +338,7 @@ export class MatCalendar<D> implements AfterContentInit, OnDestroy, OnChanges { | |||
| this._dateAdapter.addCalendarMonths(this._activeDate, 1); | |||
| break; | |||
| case ENTER: | |||
| if (this._dateFilterForViews(this._activeDate)) { | |||
| if (this.dateFilter(this._activeDate)) { | |||
There was a problem hiding this comment.
do we need to check the min and max here too? or will we never wind up in that situation?
There was a problem hiding this comment.
I'll check it more carefully. I've just replaced it without considering this possibility.
There was a problem hiding this comment.
In fact I couldn't imagine a scenario where the user is able to select a disabled date with keyboard.... as soon as I set max and min dates inside <month-view>, and I've just noticed that I forgot to do it. 😳
One option would be keeping the _dateFilterForViews and don't pass maxDate and minDate to <month-view>, but it would be potentially confusing in the future (why only the filters of month-view would be in calendar.ts?) To keep it consistent, all of filtering logic should follow the same rule. So each view has all its filtering logic inside itself.
I can go on the other way if you think it's better: keep all the filtering logic in calendar.ts (and don't pass maxDate and minDate to the views components). Let me know if you prefer this option.
There was a problem hiding this comment.
Oh wait this is coming back to me now, I think what happened is that I allowed people to navigate to disabled dates with the keyboard for the following reason: imagine someone created a date filter that returns date < 1/1/2018, now if I try to arrow forward from 2017 and go to the next enabled date... my application hangs forever. So instead I just made it impossible to select the date if the active date is disabled.
So I think we do need to make sure we somehow check the min and max dates. We should actually be doing it for year and multi-year view too, I think it's a bug that we're currently not. I feel like the way this should work is that the calendar should delegate the keyboard handling to the appropriate view rather than having the code for all of the views hardcoded in the calendar, but its already kind of set up weird, so however you want to make it work is fine...
There was a problem hiding this comment.
Moving each keyboard handler to its own view would be nice, but it would also add a delay to this PR, so what if we do it in a separate one? I think the performance issue that lead to this PR is really anoying and should be merged ASAP.
There was a problem hiding this comment.
Edit: ok after looking over the code some more you've convinced me that this is actually right. It looks like dates that are disabled via the date filter can be activated by the keyboard, but dates beyond the min and max can't so this should be safe. And as you pointed out we intentionally allow selecting disabled dates in year/multi-year view, so those ones should be right too. Sorry for the confusion.
There was a problem hiding this comment.
We have @Input() allowDisabledSelection in mat-calendar-body. It's used in <year-view> and in <multi-year-view>. We could see whether it can be used in <month-view> too, and get rid of the ENTER key filter logic... in a future PR.
mmalerba
added
pr: lgtm
action: merge
|
@mmalerba, I'll start the refactoring that takes each keyboard event handler to inside of the appropriate view. It will be a new PR that I'll start on top of this one.
edit: little mod => I had forgotten to add a protection to an undefined filter. Just fixed. |
* add max/min filter to multi year and year views * address @mmalerba's comments * fix and tighten up logic * possibility to have a null filter in month view
|
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. |
Improves the performance when open multi year and year view with max and/or min dates specified.
Closes #9725.