refactor(datepicker): use ng-if to increase performance

[Foxandxss]

It is going to be fun, someone said. Damn, I never felt this much pain on a refactor.

Adding an extra scope in the middle of this directive was way problematic.

Adding that ng-if needed to modify all the popup tests to manually open the datepicker (earlier, you could run your tests vs a hidden datepicker but not if it is not there).

On the other hand, I needed to use the dot.rule for the date so it would work as expected.

On the $destroy $popup is the comment of the ng-if, so you have to destroy the sibling element.

What I am not happy about is the close button. Since there is a new parent scope, you need an extra $parent. Can I use there the dot.rule? Sure but that would need an extra $watch.

Let me explain: If I change everything like: s/scope.isOpen/scope.popup.isOpen/ it would work, but I still need to $watch if the isOpen changed outside.

Said this. Feel free to review it and suggest changes.

[bekos]

It is going to be fun, someone said. Damn, I never felt this much pain on a refactor.

You probably have never refactored any legacy code! Changing in 4 lines the scope.date to scope.popup.date, you call this pain? :-) About the tests, instead of modifying each test you should identify which are testing with the calendar initially closed and create a separate describe('with initially closed'). For the rest you should either use the is-open="true" or use the open function only in the beforeEach.

For the close button it's better to create a close function instead of chaining the $parent properties.

BTW I am working on datepicker's accessibility (https://github.com/bekos/bootstrap/tree/dt_esc) so I am not going to merge this before because the changes there a lot more painful ;-)

[Foxandxss]

Changing the code itself wasn't pain. The test were :P

About the tests, more or less is what I did. On those describe I could just put is-open I did, the others are opened manually.

About the close, yes, it sounds stupid but I didn't thought about that... Sometimes the easier solution is the good one.

Finish your accessibility stuff, I will rebase when you're done :)

[bekos]

@Foxandxss BTW the open on focus for calendars, is considered bad practice for accessibility, so it will be removed as default behavior (probably, will add another directive)

So, it 's better to change the code you use to open the calendar.

[Foxandxss]

Well, the focus thingy was copied from your tests, so I used that. What should we use for opening the datepicker no? just playing with is-open?

[bekos]

@Foxandxss #1922 has landed. Can you rebase this?

[Foxandxss]

Yes, I'll do that shortly.

[Foxandxss]

I am going to redo it, there is a loot of changes and the rebase is more PITA than redoing it.

[Foxandxss]

@bekos What are we going to do with the focus to open? I saw you deleted the test, so I don't know what is the good way to test that it opens (which is not tested ATM AFAIK). I'll try to just test for not open and open, but would be good to test the opening itself. (to move from no-dom to dom and then to no-dom again).

[Foxandxss]

Ok, done.

There is a fixed indent also.

[bekos]

@Foxandxss I tried to play with this and I noticed there are some issues regarding the focus management. Unfortunately, there are no unit tests for this. It would be nice if you can confirm this and come up with a fix.

[Foxandxss]

I'm back from my tethering madness.

What you mean with the focus management? I don't recall using focus on my tests.

Is there any problem? If so, let me try.

[Foxandxss]

Talking about focus, we need to bring it back @bekos . A lot of users are complaining now because they expect a datepicker-popup to be opened with a click, which I find normal.

We need to find a way to make it accessible.

[RobJacobs]

+1 for this. Made a huge difference in performance.

@Foxandxss I'm seeing an issue with this change where the datepicker popup closes when I click on the internal month/year picker. I'm using a build from:
https://github.com/Foxandxss/bootstrap.git - nfifpicker branch.

_EDIT_
I solved this problem by changing the datepickerPopupWrap directive from binding the element click event to stop the event propogation to a function:

        scope.preventClick = function (event) {
            event.preventDefault();
            event.stopPropagation();
        };

Then adding ng-click='preventClick($event)' to the datepicker popup template.

[skidvd]

This fix in combinations with the preventClick and ngAnimate workaround (added wrapping span) drastically improve original scope and watch increases and overall performance. Before fix, I was seeing 60 added scopes and 302 added watches for each datepicker with popup. After fix, I see 2 added contexts and 8 added watches per picker. Nice improvement!

One unfortunate consequence of the fix in that the popup does not stay in synch with subsequent changes to the date (whether made through the text field or the popup). This is assuming that the new selected date is on a different year or month than the popup originally started with. Prior to these changes, this was not the case and it stayed in synch automatically. Anyone else run in to this and/or have a suggested fix?

[wesleycho]

Moved milestone back, not a high priority right now for getting 0.13 out.

[karianna]

Also can't be merged at present, see conflicts.

[Foxandxss]

Conflicts don't worry me that much. I will rereview it again, check the scope issue and then discuss the merge

[skidvd]

I would appreciate it if you would also consider my Feb 18 comment related to one unfortunate side affect in the process as well please.

[Foxandxss]

@skidvd yes, that is what I meant with "scope issue" :P

[skidvd]

Any idea when this may make it into a released version? As it was deemed as 'not high priority', I am wondering what this means in terms of when it is likely to be released. Wondering if I need to consider alternative implementations or approaches in the meantime.

[Foxandxss]

I Will merge it on 0.13.1. There is no other approaches as today. This is a massive unpaid project so you will have to bear with us. We are working as fast as possible.

[skidvd]

@Foxandxss Thanks! I understand and I was not trying to create any pressure or complain - I just need to understand rough plans for our planning purposes.

[RobJacobs]

@skidvd I've seen people use an inline datepicker wrapped in their own element with ng-if as a work around.

[rvanbaalen]

@Foxandxss Can you wrap this up? Let's get it over with.

[RichardJECooke]

Hi everyone. Is this fix in the latest release version (0.13.0)? http://angular-ui.github.io/bootstrap/

I've been using a dozen 0.12 date pickers on a page and Batarang shows them as the top 5 watches taking entire seconds of time to do nothing (Angular CSS class watches mostly). Will this fix help that problem? I don't really understand what it's doing.

[kj1981]

Is there any working solution that works with Angular 1.3.4, NgAnimate 1.3.0, angular-ui-bootstrap/0.13.4
Sorry but at this moment I cannot update these libraries so wanted to know if there is a workaround/hack for this because I am struggling to find solution or alternative. Below is my codepen if it helps: https://codepen.io/anon/pen/wEaoLG