feat(datepicker): export NgbDatepickerMonthView component #3396

gpolychronis · 2019-10-04T10:23:32Z

Export NgbDatepickerMonthView component.
This component corresponds to the view of the days of a single month of the datepicker.
This export is an attempt to allow:

Use of the month view without the rest datepicker visual elements.
Customization of how a month is displayed inside datepicker i.e. add a header title per month.

Code has been refactored, so that:

NgbDatepickerMonthView's only input is the year and month to be displayed.
NgbDatepicker can have a custom content. Combined with the export of NgbDatepickerMonthView it allows to customize datepicker with new DOM elements per month.

NgbDatepickerMonthView remains linked to an instance of a NgbDatepicker, to support the various functionalities it is accompanied with (i.e. i18n, focus, keyboard navigation etc.)

gpolychronis · 2019-10-04T10:24:03Z

@benouat @maxokorokov @ExFlo
Please review

maxokorokov · 2019-10-04T12:46:45Z

@gpolychronisamadeus haven't looked at it yet, but the build is failing (will restart the build to see if it wasn't a fluke)

Also some description of what you're trying to do, why and providing some examples is always helpful to review.

UPDATE: build worked, was a sauce labs fluke apparently.

codecov-io · 2019-10-04T13:13:03Z

Codecov Report

Merging #3396 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3396      +/-   ##
==========================================
+ Coverage   91.71%   91.74%   +0.02%     
==========================================
  Files         100      100              
  Lines        2898     2907       +9     
  Branches      535      537       +2     
==========================================
+ Hits         2658     2667       +9     
  Misses        183      183              
  Partials       57       57

Flag	Coverage Δ
#e2e	`56.62% <87.5%> (+0.06%)`	⬆️
#unit	`88.53% <100%> (+0.03%)`	⬆️

Impacted Files	Coverage Δ
src/index.ts	`100% <ø> (ø)`	⬆️
src/datepicker/datepicker.module.ts	`100% <ø> (ø)`	⬆️
src/datepicker/datepicker-service.ts	`97.5% <100%> (+0.08%)`	⬆️
src/datepicker/datepicker-keyboard-service.ts	`100% <100%> (ø)`	⬆️
src/datepicker/datepicker.ts	`98.11% <100%> (+0.11%)`	⬆️
src/datepicker/datepicker-month.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddc4ceb...8ed67e2. Read the comment docs.

maxokorokov

Hey, @gpolychronisamadeus!

Looks promising, thanks!

Big things that we should address:

let's remove NgbDatepickerMonth and replace it with {month: number, year: number, day?: number} for public API
maybe try move registerMonthView implementation completely from the datepicker to the NgbDatepickerMonthView ?
keyboard support not working for custom content template
demo from the overview not working when I copy-paste it
missing integration tests (ex. to check keyboard with custom template)
we also have NgbInputDatepicker that should support this functionality, don't forget about it. We could do it in a separate PR, will just need to make sure it will work

Let's also in future support datepikcer.state.months[] to get generated months directly!

src/datepicker/datepicker-month-view.ts

src/datepicker/datepicker-service.ts

src/datepicker/datepicker.ts

demo/src/app/components/datepicker/overview/datepicker-overview.component.ts

src/datepicker/ngb-date-struct.ts

gpolychronis · 2019-11-15T09:28:57Z

@maxokorokov
I will rebase current PR upon #3332 to use the state.
However, this means that this PR effectively is dependent on 3332, and cannot be reviewed before it has been merged.

maxokorokov · 2019-11-15T16:03:32Z

@gpolychronisamadeus I think it should be safe to rebase this on master now

gpolychronis · 2019-11-15T16:07:45Z

@gpolychronisamadeus I think it should be safe to rebase this on master now

@maxokorokov
keydown event is broken in my local rebased branch. After I figure out why and fix it I will push again.

gpolychronis · 2019-11-20T16:30:48Z

* [ ]  we also have `NgbInputDatepicker` that should support this functionality, don't forget about it. We could do it in a separate PR, will just need to make sure it will work

Will create a different PR or provide an alternative via demo example. Let's discuss on this.

maxokorokov

Hey, @gpolychronisamadeus!

As discussed there is a severe performance regression when changing months with keyboard when multiple months are visible → both memory impact and JS processing impact:

Before:

After:

You need to check where handlers are allocated / GCed, but especially DOM nodes. Looks like they're not reused as before.

Some ideas we had:

don't override the whole content, but only the inside of the <div class="dp-months">
fix focusin/out handlers

Also left some comments:

KeyboardService → Calendar dependency might not work as expected
please check your code for unused imports, they're everywhere

maxokorokov · 2019-11-29T10:09:39Z

demo/src/app/components/datepicker/demos/keyboard/datepicker-keyboard.ts

    const state = dp.state;
    switch (event.code) {
      case Key.PageUp:
-        dp.focusDate(calendar.getPrev(state.focusedDate, event.altKey ? 'y' : 'm'));
+        dp.focusDate(this.calendar.getPrev(state.focusedDate, event.altKey ? 'y' : 'm'));


Typescript is pretty unhappy with this.calendar for some reason. Not sure why

maxokorokov · 2019-11-29T10:11:08Z

src/datepicker/datepicker-keyboard-service.ts

@@ -10,18 +10,22 @@ import {Key} from '../util/key';
 */
 @Injectable({providedIn: 'root'})
 export class NgbDatepickerKeyboardService {
+  constructor(private _calendar: NgbCalendar) {}


As KeyboardService provided in root, I wonder what will get injected when the calendar is overridden at the datepicker component level ?

I guess it should be left as an argument to the processKey function

src/datepicker/datepicker-service.ts

maxokorokov · 2019-11-29T10:18:42Z

src/datepicker/datepicker-service.ts

@@ -151,6 +151,15 @@ export class NgbDatepickerService {
    return this._calendar.isValid(ngbDate) ? ngbDate : defaultValue;
  }

+  getMonth(struct: NgbDateStruct) {
+    for (let viewModel of this._state.months) {


nit: maybe month instead of viewModel?

It is the input of NgbDatepickerMonthView. User might provide a an object with just the year and the month and not a proper NgbDate object.

it was a nitpick and I was talking purely about the viewModel variable name

src/datepicker/datepicker.ts

maxokorokov · 2019-11-29T10:19:49Z

src/datepicker/datepicker.ts

@@ -91,6 +92,19 @@ export interface NgbDatepickerState {
   * The date currently focused by the datepicker
   */
  readonly focusedDate: NgbDate;
+
+  /**
+   * The months in range of the datepicker


need more details here, not clear that dates are first dates for month

maxokorokov

Hey, @gpolychronisamadeus!

I'd very much like to finish with this, but I've still seen these issues:

performance. It's better now, but still not acceptable and needs more investigation (I'll try to allocate some time for it too)

Normal datepicker with 2 months:

Customised template with 2 months:

120Mb and 80K nodes still look excessive. Looks like they're garbage collected, but I'd like to understand the difference with the normal implementation.

please rebase
left a comment about keyboard

demo/src/app/components/datepicker/demos/keyboard/datepicker-keyboard.ts

gpolychronis · 2020-01-10T15:29:32Z

@maxokorokov
I wait until #3530 (comment) is closed.

I think this might help with performance.

gpolychronis · 2020-02-03T14:27:59Z

As discussed the following changes will have to be done:

Replace month of NgbDatepickerMonthView with setter.
Put onKeyDown on host.
Use language or (existing's) library find method in getMonth. Check if supported by all browsers first
Make contentTemplate static = true

Documentation issue will be fixed by an other commit.

maxokorokov

@gpolychronis one last small change → you changed the NgbDatepicker constructor arguments for no reason, could you please rollback and I'll mark as to merge ?

src/datepicker/datepicker.ts

src/datepicker/datepicker-keyboard-service.ts

maxokorokov · 2020-02-14T10:43:57Z

As we've discussed:

removed the additional div element that was added for months
renamed ngb-datepicker-month-view to ngb-datepicker-month
updated the custom month layout demo with navigation example

Will merge when green

maxokorokov added component: datepicker needs: review type: feature labels Oct 4, 2019

maxokorokov added this to the 5.2.0 milestone Oct 22, 2019

maxokorokov added needs: work and removed needs: review labels Nov 14, 2019

maxokorokov requested changes Nov 15, 2019

View reviewed changes

maxokorokov added the needs: tests label Nov 15, 2019

maxokorokov added needs: review and removed needs: work labels Nov 15, 2019

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch from 9f53422 to 45cc80f Compare November 20, 2019 16:29

maxokorokov added needs: work and removed needs: review needs: tests labels Nov 29, 2019

maxokorokov requested changes Nov 29, 2019

View reviewed changes

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch from 45cc80f to f5486ee Compare November 29, 2019 15:50

maxokorokov added needs: review and removed needs: work labels Nov 29, 2019

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch 2 times, most recently from b4fcb49 to e057fb9 Compare December 4, 2019 14:24

maxokorokov requested changes Jan 10, 2020

View reviewed changes

demo/src/app/components/datepicker/demos/keyboard/datepicker-keyboard.ts Show resolved Hide resolved

maxokorokov added needs: work and removed needs: review labels Jan 10, 2020

maxokorokov modified the milestones: 5.2.0, 5.3.0 Jan 10, 2020

gpolychronis mentioned this pull request Jan 23, 2020

fix(datepicker): focus on multiple instances (#3494) #3549

Merged

5 tasks

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch from e057fb9 to be44445 Compare January 29, 2020 09:07

maxokorokov added the needs: review label Jan 29, 2020

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch from be44445 to 35b0403 Compare February 4, 2020 08:31

maxokorokov removed the needs: review label Feb 4, 2020

maxokorokov approved these changes Feb 7, 2020

View reviewed changes

src/datepicker/datepicker.ts Outdated Show resolved Hide resolved

src/datepicker/datepicker-keyboard-service.ts Outdated Show resolved Hide resolved

maxokorokov added needs: merging needs: rebasing and removed needs: work labels Feb 7, 2020

gpolychronis force-pushed the datepicker/export-NgbDatepickerMonthView branch from 35b0403 to 693bf0b Compare February 7, 2020 11:21

maxokorokov removed the needs: rebasing label Feb 13, 2020

gpolychronis-amadeus and others added 2 commits February 14, 2020 11:28

feat(datepicker): export NgbDatepickerMonthView

3bc1a0f

demo(datepicker): beautify custom month layout demo

8ed67e2

maxokorokov force-pushed the datepicker/export-NgbDatepickerMonthView branch from 693bf0b to 8ed67e2 Compare February 14, 2020 10:40

maxokorokov merged commit 9941457 into ng-bootstrap:master Feb 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datepicker): export NgbDatepickerMonthView component #3396

feat(datepicker): export NgbDatepickerMonthView component #3396

gpolychronis commented Oct 4, 2019 •

edited

gpolychronis commented Oct 4, 2019

maxokorokov commented Oct 4, 2019 •

edited

codecov-io commented Oct 4, 2019 •

edited

maxokorokov left a comment

gpolychronis commented Nov 15, 2019

maxokorokov commented Nov 15, 2019

gpolychronis commented Nov 15, 2019 •

edited

gpolychronis commented Nov 20, 2019

maxokorokov left a comment •

edited

maxokorokov Nov 29, 2019

maxokorokov Nov 29, 2019 •

edited

maxokorokov Nov 29, 2019

gpolychronis Nov 29, 2019

maxokorokov Nov 29, 2019

maxokorokov Nov 29, 2019

maxokorokov left a comment

gpolychronis commented Jan 10, 2020

gpolychronis commented Feb 3, 2020 •

edited

maxokorokov left a comment

maxokorokov commented Feb 14, 2020 •

edited

feat(datepicker): export NgbDatepickerMonthView component #3396

feat(datepicker): export NgbDatepickerMonthView component #3396

Conversation

gpolychronis commented Oct 4, 2019 • edited

gpolychronis commented Oct 4, 2019

maxokorokov commented Oct 4, 2019 • edited

codecov-io commented Oct 4, 2019 • edited

Codecov Report

maxokorokov left a comment

Choose a reason for hiding this comment

gpolychronis commented Nov 15, 2019

maxokorokov commented Nov 15, 2019

gpolychronis commented Nov 15, 2019 • edited

gpolychronis commented Nov 20, 2019

maxokorokov left a comment • edited

Choose a reason for hiding this comment

maxokorokov Nov 29, 2019

Choose a reason for hiding this comment

maxokorokov Nov 29, 2019 • edited

Choose a reason for hiding this comment

maxokorokov Nov 29, 2019

Choose a reason for hiding this comment

gpolychronis Nov 29, 2019

Choose a reason for hiding this comment

maxokorokov Nov 29, 2019

Choose a reason for hiding this comment

maxokorokov Nov 29, 2019

Choose a reason for hiding this comment

maxokorokov left a comment

Choose a reason for hiding this comment

gpolychronis commented Jan 10, 2020

gpolychronis commented Feb 3, 2020 • edited

maxokorokov left a comment

Choose a reason for hiding this comment

maxokorokov commented Feb 14, 2020 • edited

gpolychronis commented Oct 4, 2019 •

edited

maxokorokov commented Oct 4, 2019 •

edited

codecov-io commented Oct 4, 2019 •

edited

gpolychronis commented Nov 15, 2019 •

edited

maxokorokov left a comment •

edited

maxokorokov Nov 29, 2019 •

edited

gpolychronis commented Feb 3, 2020 •

edited

maxokorokov commented Feb 14, 2020 •

edited