refactor(datepicker)

[bekos]

I will start a PR thread for input/comments, while I am refactoring the datepicker directive.
Main goal is:

• Split into smaller directives (dayPicker, monthPicker, yearPicker and be easy to add more)
• Move common logic shared by directives into controller.
• Address issues with min, max, initial selection, repeated dates etc (TODO: reference #issues)

[chrisirhc]

I was playing around with this and I realized an alternative maybe to use getter methods for these scope variables that need to fallback to default values if undefined.

E.g.
getCloseText: function () { return scope.closeText || datepickerPopupConfig.closeText }

Of course, they can be shortened and generalized if they're the same keys on the scope and config objects.

Just a suggestion / thought.

[bekos]

I see. Is there an issue for this in AngularJS or is it the expected behavior?

[chrisirhc]

Hm.. not sure what you mean. I was referring to the switch from $observe to this scope.template.
This current change breaks the binding of the currentText (perhaps that's intended).
However, if you do want to bind it, one solution is to use a getter method to fetch it from the scope (in the template, it would be a function call like {{ template.getCloseText() }} ... or {{ get('closeText') }} if we use a generic getter ).

The getter method can enforce the defaults. This way, if the scope variable is changed to undefined, it still falls back to the datepickerPopupConfig value.

[bekos]

I want to break the binding. No need to obsereve these changes in the default behavior.
What I mean is that last night I tried:

scope.closeText  = scope.closeText || datepickerPopupConfig.closeText;

in the tempate the {{closeText}} was evaluated again from the attribute, ie null, didn't get the default value.
That's why I assign in a different variable.

I hope this makes sense :-)

[chrisirhc]

Ah okay, that null thing is a bit of a pain. A way around it is perhaps to use evalAsync but I think using a getter function would also solve that.

[bekos]

Yeah, but this doesn't seem to be like the expected behavior, that's why I'm asking if you know if there is an issue for this in AngularJS.

[chrisirhc]

I believe this is expected behavior because using scope attributes (scope: { closeText: '@' , .. }) just adds watchers (with $observe) to the attributes. The watchers initial run is done asynchronously (similar to evalAsync, I think), so they have not run yet at this point.

I don't know if there's an open issue on this and I'm about to head to sleep so I won't be able to dig it out for you heh.

[bekos]

No problem :-)

[Foxandxss]

I like the changes so far. Datepicker is a monster, both test a code.

[Foxandxss]

I guess this is the best place (better than a new issue) to discuss an idea I had in mind.

Typically the calendars / datepickers are 7x6 and this one goes between 4 and 6 rows. I think that it would be better if we fix it to 6 rows. That would help designers I guess, to not have a jumpy element when moving through months. On the other hand, someone could like to be able to see some of the past/next month days.

I see this behavior in practically any calendar, windows, mac, mobile...

Since the date generator algorithm is that awesome (I am stealing it to make my app better, sorry), it is just changing a couple of numbers to make this change.

Not sure what's the opinion of the community :)

[bekos]

The refactor is almost done. It is still missing some tests, mainly for the new features, documentation changes and other small tweaks, but the main logic is already there. I will leave it here for some days in case someone wants to make a code review.

A few key-points off the top of my head.

Breaking changes:

• show-weeks is not $watched.
• *-format attributes have been renamed to format-*
• min/max have been renamed min-date/max-date

Features:

• added datepicker-mode as optional two-way binded attribute. Use: initialize datepicker in specific mode and watch/set mode outside of the datepicker
• added init-date to specify initial view when model is not set
• added visual hint for current day, month and year.

Fixes:

• Repeated dates issue

Internals:

• Split into separate directives with unique templates. This also helped in better separation of concerns.

TODO (in following PRs):

• use ng-if for popup directive that will increase performance significantly.
• use ngAnimate for transitions between modes and opening/closing (with help by @chrisirhc :-) )

[bekos]

@Foxandxss I hear what you say and I agree that this is probably a better experience, as you say. I think it's an easy fix. You can open a PR after this one has landed :-)

[Foxandxss]

It should be easy to fix and test. I'll check your changes shortly :)

[tomchentw]

@Foxandxss +1 for fix it to 6 rows :)

[Foxandxss]

@bekos I have some thoughts:

Isn't selected a bad name choice? People could get confused with the ngModel date and selected date. I would rename it to selectedCalendar or calendarDate (something like that). It would help reading the code and understanding it.

Otoh I have a question myself: Why is the ngModel put on a parent? It is a weird usage imho. Normally you only need to put your directive, but having to wrap it on an element with a ngModel is weird. What were the needs to do that?

[bekos]

@Foxandxss I agree about the selected, I will rename to something more understandable.

The parent was a workaround before 1.2, and the issue with isolated scopes (angular/angular.js@909cabd). I have changed the demo now.

[Foxandxss]

If we are supporting an optional ngModel, this line seems to cry when we don't have any ngModel assigned. Probably there are more spots. Maybe we need a battery of tests for when there is no ngModel? :)

[bekos]

Yes, you are right. Let me land this and open a PR later for the optional ng-model fixes.

[Foxandxss]

I think I have some kind of love with this directive :P

Have you thought about putting all the pieces in different files, that would ease the hacking and maybe opening a door for new features.

I got a guy at IRC who wanted a multi day datepicker. Even when the idea was discarded (no enough knowledge to hack it himself) that gave me the idea. We could split it in different files so someone can just fork daypicker.js to add some changes and when we update, he just need to maintain one small file instead of a big big one.

On the other hand that could open a door to add a multidaypicker.js some day or other kind of datepicker related directives.

[Foxandxss]

Uhh, merged. What does remain to do here @bekos ? The non-model fix and the 42 days calendar, right?

I will work on the 42 days calendar soon and PR it to discuss it.

[bekos]

Other things TODO:

• use ng-if for popup directive that will increase performance significantly when multiple instances exist on the same page.
• use ngAnimate for transitions between modes and opening/closing
• add keyboard navigation
• fix to 7 weeks per month (I am not sure if this should be an option)
• fix Datepicker today button sets time #1726
• fix missing ngModel scenario

(ping @tomchentw)

[Foxandxss]

There is a weird behavior that I am not sure how to fix. When in a given month you pick on a date of the next month, depending on the next month layout, you will get that new date selected but the button you used to select is still focused.

[bekos]

@Foxandxss Thx for noticing. The issue was the wrong track by expression (#1771)