Datepicker directive issues

[guinnberg]

Hi all,

I've started using this directive, and I can see some inconsistency between options from datepicker and the popup version.

In addition, and it's just my opinion, the code looks over complicated for the functionality it has, so I think it'd be worthy a refactor of it.

Nevertheless there is one very critical issue with this directive. I'm using the popup version in a web page with more than 50 fields (hidden by default, but they're there), and every time we use the directive a new datepicker element is appended so this is slowing down too much the rendering phase.
I think it shouldn't be that difficult, just create one date picker for popups and reuse it all along the textboxes which call it...

So, if you really think it's worthy, I can try to do my best to improve this directive, but you know... it'll be a huge work and I don't wanna spend several hours on doing it and then get that you have in mind either a better solution or another kind of refactor.

Cheers!

[bekos]

@ivogallego Any help or idea you may have that will make datepicker or any other directive simpler is more than welcome.

Atm there are some open PRs about the datepicker but none of them is really changing the core of this directive. They are simple enhancements or fixes. The only idea so far is by @pkozlowski-opensource, to split into inner directives like daypicker, monthpicker etc. but you can come up with something better or just by refactoring the existing code base.

About the one instance datepicker popup, first of all I would like to know the use case of a page with 50 date inputs :-) but I think it's a nice proposition. If you can also replicate the behavior of multiple calendars, (ie keeping state) that would be even better.

Also, can you spend some more details about the inconsistencies in the popup options? Do you mean the json on the datepicker-options?

Anyway, I really appreciate any help / discussion / PR / whatever for this directive :-)

[guinnberg]

@bekos could you tell me in what issue @pkozlowski-opensource is discussing about that? Maybe I can get or give more ideas based on his ones...

I'm having those 50 calendars, because we have a huge questionnaire, and depending on your answer you need to provide a date. Right now I improved that a little bit using ng-switch, but like we're still using Angular 1.0.8 I cannot user ng-if, so I'm also using ng-show.

About the thing of the multiple calendars, I don't know if I'm understanding you properly (I'm not native English as you can guess :P), but you mean to have multiple, just say, textboxes and all share the same value of the calendar? If this is it I don't see it very useful, but shouldn't be difficult to do either.

What I was saying about the inconsistency is the datepicker-options, yes. I didn't try all of the functionalities, but I spent some time trying to set the show-week to false, and I found this in the code inside the datepickerPopup directive:

datepickerEl.attr(angular.extend({}, originalScope.$eval(attrs.datepickerOptions)));
At this point the json specified is being set to the datepicker element, which is ok, but few lines below...

if (attrs.showWeeks) {
  addWatchableAttribute(attrs.showWeeks, 'showWeeks', 'show-weeks');
} else {
  scope.showWeeks = true;
  datepickerEl.attr('show-weeks', 'showWeeks');
}

Since I'm not specifying any attribute at the pop up level but the datepickerOptions, attrs.showWeeks is undefined. I just added in my local another else condition to check if the element already has that attribute. I think it could be done in a better way, I'd should spend a bit of time to be sure, but if a refactor is done, maybe we don't need to have this fix, because of this I didn't do any PR.

[bekos]

@ivogallego There was no extended discussion, but the concept is that ta datepicker will switch bwtween the daypicker, monthpicker & yearpicker directives.

I am not a native English speaker, also, but what I wanted to say is that when you have multiple instances each one can have a different browsing state. For example, you can browse to the next year, open another calendar, and when returning back to first one, it opens one year later as you left it. Probably not a crucial feature, in contrast to performance gain anyway.

About the options feel free to submit the PR. The refator about the inner directives is about the datepicker, not the datepicker-popup.

[guinnberg]

Alright @bekos, I'll send a PR later on today if I have time, if not I'll do it along the weekend.

Also in the weekend, I'll have a deep look to the whole directive and see if I can contribute with something else. Btw, what are the reasons to divide the directive in 3 subdirectives? That's what I wanted to see in the discussion :P

[bekos]

If you split each mode into it's own directive you can move the code of this.modes into each own directive and also have a separate template for each mode.

The advantage is that code will be easier to understand and maintain and user will be able to override the template per mode.

[bekos]

The #924 is heading this way, so each inner directive can use the common logic through the controller. I suggest taking a look.

[pkozlowski-opensource]

I think that @bekos covered most of the items already, just my 2 quick comments:

• @ivogallego we are always looking for possible code improvements, but just go over the code before jumping to conclusions. It is easy to underestimate complexity of existing use-cases expressed in the unit tests in place. Take a stab at refactoring, you've got safety net of tests and send a pull request with the improvements - we will be happy to review.
• @bekos already well summarized our brief discussion on potential simplifications to the directive. Smaller, more focused directives are easier to test, reason about etc.

I guess this issue can be closed for now, fell free to send a pull request with the improvements you might come up with.

[guinnberg]

Hi @pkozlowski-opensource,don't get me wrong I didn't meant to blame anybody...

If I opened to prior to even think of a refactor is because I already spent my time twice on PR which never being merged either because of someone was already doing it or because it didn't match some expectations so I wouldn't like to waste my time again.

As you can read I never say that it can be really improved but that's my feeling and actually I was thinking to review the tests as well because I guess they aren't covering at least the thing I said about the show weeks

[pkozlowski-opensource]

@ivogallego I understand, no hard feelings. It is just that is much easier to discuss about code in PR than about feelings. And we can't guarantee that all the PRs will be merged, this is the "risk" we all trying to contribute to a project. Personally I can't count how many times I've scraped my own PRs for this very project :-)

So, if you've got good stuff, bring it in!

[guinnberg]

I understand and agree, we'll discuss it again in the PR ;-)