-
Notifications
You must be signed in to change notification settings - Fork 154
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
Calendar pipe: wrap setTimeout with outsideAngularApp #135
Conversation
- make initTimer to be not static anymore
- pass NgZone and ChangeDetectorRef as parameters to InitTimer method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I reviewed the changes, here are some comments
src/calendar.pipe.ts
Outdated
CalendarPipe.removeTimer(); | ||
CalendarPipe.initTimer(ngZone, cdRef); | ||
ngZone.run(() => cdRef.markForCheck()); | ||
}, timeToUpdate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to run the markForCheck()
here - we emit an event on the midnight
emitter, and the constructor subscribes and runs the change detector. Since there can be several change detector refs, it makes sense to keep this call inside the constructor.
Also, I think we can remove the runOutsideAngular()
from the constructor itself - it seems to be unnecessary left over from times when the timer was created directly inside the constructor.
- remove change detection from InitTimer method
Done :) |
Sorry, one more fix. Return a timer from runOutsideAngular method to assign it to the CalendarPipe.timer |
thanks, looks good! |
released as 1.3.3 |
amCalendar causes protractor to timeout on waiting async Angular #132