perf(ripple): do not register events if ripples are disabled initially #8882
Conversation
googlebot
added
the
cla: yes
src/lib/core/ripple/ripple.ts
Outdated
| // The events for a trigger element shouldn't be registered if ripples are disabled. | ||
| // As soon as the ripples are enabled again, the events for the current trigger will be added. | ||
| if (!this.disabled && this._isInitialized) { | ||
| this._rippleRenderer.setTriggerElement(trigger); |
There was a problem hiding this comment.
It feels strange to call setTriggerElement when you really just want one of the side-effects of setTriggerElement. Would it be easier to follow if there was a function to specifically add/remove the event listeners?
There was a problem hiding this comment.
What do you mean with side-effects? Maybe the function name is just not good and it should be called registerTriggerEvents() or something like that?
There was a problem hiding this comment.
Based on the name, the primary purpose of setTriggerElement would be the line
this._triggerElement = element;The event stuff is incidental to updating the trigger element. It be a bit better as something like
/** Sets the trigger element and registers the mouse events. */
setTriggerElement(element: HTMLElement | null) {
if (this._triggerElement === element) {
return;
}
this._removeTriggerListeners();
this._triggerElement = element;
this._addTriggerListeners();
}Then other call site that don't need to update to a new element just call _removeTriggerListeners and _addTriggerListeners
There was a problem hiding this comment.
I see what you mean, but the actual event registration should be the primary purpose of this function (or part in the RippleRenderer)
In that case I would just call the method _setupTriggerEvents() and keep the implementation as it is. The _triggerElement variable is just there for the event un-registration, and the real trigger element is stored in the MatRipple class. Does that work for you?
There was a problem hiding this comment.
Agreed that at least the naming should change to something involving "events".
| @@ -229,6 +228,7 @@ export class MatTabLink extends _MatTabLinkMixinBase | |||
| // Manually create a ripple instance that uses the tab link element as trigger element. | |||
| // Notice that the lifecycle hooks for the ripple config won't be called anymore. | |||
| this._tabLinkRipple = new MatRipple(_elementRef, ngZone, platform, globalOptions); | |||
| this._tabLinkRipple.ngOnInit(); | |||
There was a problem hiding this comment.
It feels weird having to call the Angular lifecycle hook manually. Shouldn't the renderer support attaching to arbitrary elements?
src/lib/core/ripple/ripple.ts
Outdated
| // because the events should be only registered if the ripples aren't disabled initially. | ||
| // Also if there is no trigger element, that has been specified explicitly through the input, | ||
| // the default trigger element will be the directive's host element. | ||
| this.trigger = this._trigger === undefined ? this._hostElement : this.trigger; |
There was a problem hiding this comment.
Doing this just so the setter runs is somewhat side-effect-ey. Why not have a private method that does the attachment both here and insider the setter?
src/lib/core/ripple/ripple.ts
Outdated
| // The events for a trigger element shouldn't be registered if ripples are disabled. | ||
| // As soon as the ripples are enabled again, the events for the current trigger will be added. | ||
| if (!this.disabled && this._isInitialized) { | ||
| this._rippleRenderer.setTriggerElement(trigger); |
There was a problem hiding this comment.
Agreed that at least the naming should change to something involving "events".
devversion
force-pushed
the
perf/ripple-events-register-initial-disabled
branch
5 times, most recently
from
2f972c6
to
75a5fe0
Compare
|
@devversion just needs rebase whenever you have time |
devversion
force-pushed
the
perf/ripple-events-register-initial-disabled
branch
from
75a5fe0
to
30fbd9e
Compare
devversion
added
target: patch
* No longer registers trigger event listeners if the ripples are disabled initially. Fixes angular#8854.
devversion
force-pushed
the
perf/ripple-events-register-initial-disabled
branch
from
30fbd9e
to
270b974
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #8854.