New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

datepicker: reduce watchers #3770

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@chrisirhc
Copy link
Member

chrisirhc commented Jun 8, 2015

Mode No. of Watchers Before Change No. of Watchers After Change
daypicker 152 28
monthpicker 35 13
yearpicker 29 11

Resolves #3451 #2613

chrisirhc added some commits Jun 7, 2015

fix(datepicker): ensure one-time binding for custom-class
One-time bindings will continue to watch until the value becomes
anything other than undefined. As such, must set this to null when it is
known, otherwise watchers will stick around.

@chrisirhc chrisirhc added this to the 0.13.1 (Performance) milestone Jun 8, 2015

}
};
}])

This comment has been minimized.

@RobJacobs

RobJacobs Jun 9, 2015

Contributor

Seems like this would be useful in other areas, could me move it to a service?

This comment has been minimized.

@chrisirhc

chrisirhc Jun 9, 2015

Member

I was thinking of using it here first then move it elsewhere only we have more uses, and when more people have looked at it, heh. Can you think of other places that might need this?
It's useful for classes that apply to one element/child rather than all, in an ng-repeat, to reduce watchers.

This comment has been minimized.

@RobJacobs

RobJacobs Jun 9, 2015

Contributor

Carousel, pagination, and typeahead are using ng-class to match an active state. I have a few directives outside of angular-ui-bootstrap that I thnk could benefit from this approach.

This comment has been minimized.

@wesleycho

wesleycho Jun 9, 2015

Member

This would be pretty nifty to release as a standalone library, although that is probably outside the scope of this PR.

@wesleycho

This comment has been minimized.

Copy link
Member

wesleycho commented Jun 9, 2015

This needs tests, but otherwise looks generally good to me.

@chrisirhc

This comment has been minimized.

Copy link
Member

chrisirhc commented Jun 9, 2015

Indeed, the new directive needs tests. I'll work on that. We can port it to other libraries later, once I'm done with adding tests. This has most gain on the default use of datepicker so it makes sense for it to have it first.

$animate.removeClass(data.lastActivated.element, clazz);
}
if (newActivated) {
newActivated.element.attr('has-been-activated', 'true');

This comment has been minimized.

@chrisirhc

chrisirhc Jun 10, 2015

Member

I'll be removing this. It was a debug statement.

fix(datepicker): remove use of ng-class to reduce watchers
Now use at most one watcher for determining which date should have what
class. ng-class adds a watcher per element containing date, while this
uses one watcher per class for all elements.

@chrisirhc chrisirhc force-pushed the chrisirhc:feature/datepicker-watchers branch from b23213d to a14c6dd Jun 10, 2015

@wesleycho wesleycho modified the milestones: 0.13.1 (npm), 0.13.2 (Performance) Jul 23, 2015

@jaskaye17

This comment has been minimized.

Copy link

jaskaye17 commented Jul 23, 2015

+1 Let's get this badboy live!

@wesleycho wesleycho modified the milestones: 0.13.2 (1.4 support), 0.13.3 (Performance) Aug 2, 2015

@wesleycho wesleycho modified the milestones: 0.13.3 (Fixes), 0.13.4 (Performance) Aug 9, 2015

@jaskaye17

This comment has been minimized.

Copy link

jaskaye17 commented Aug 12, 2015

This PR looks good in terms of reducing the number of watchers. The issue that I see for many users is that the popup is always rendered even when not visible. The display is simply set to none. This leaves all of the watchers. When using multiple datepickers on a single page (or in a grid as some users have done) the number of watchers still becomes unwieldy even with the above reduced number of watchers. Are there any plans to conditionally render (ng-if) the popup when clicked.

I see that this ticket does in fact accomplish the task of reducing the watcher count on the datepicker and is not exactly tailored to the use case of multiple watchers. I ask here, however, because all datepicker watcher related tickets seem to lead me here

@RobJacobs

This comment has been minimized.

Copy link
Contributor

RobJacobs commented Aug 12, 2015

@jaskaye17 ng-if was added to the datepicker popup under this commit: b72efed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment