refactor(connected-overlay-directive): avoid issues with property initialization #3228
Conversation
googlebot
added
the
cla: yes
|
@crisbeto This appears to break md-select panel positioning. Can you fix? |
|
Fixed @kara. I don't know if calling |
crisbeto
force-pushed
the
3225/overlay-setter-refactor
branch
from
b38e051 to
66bd641
Compare
|
|
||
| ngOnChanges(changes: SimpleChanges) { | ||
| if (changes['open']) { | ||
| changes['open'].currentValue ? this._attachOverlay() : this._detachOverlay(); |
There was a problem hiding this comment.
After you check changes['open'], shouldn't you be able to just use this.open?
(same for others)
src/lib/select/select.ts
Outdated
| } | ||
|
|
||
| this._checkOverlayWithinViewport(maxScroll); | ||
| this._changeDetectorRef.detectChanges(); |
There was a problem hiding this comment.
Add comment explaining why this detectChanges call is necessary
| this._position.withOffsetX(offsetX); | ||
| } | ||
| } | ||
| @Input() offsetX: number = 0; |
There was a problem hiding this comment.
How significant is the code size difference? We use quite a lot of getter/setters.
There was a problem hiding this comment.
A getter/setter combination transpiles to this in ES5:
Object.defineProperty(A.prototype, "thing", {
get: function () {
return this._thing;
},
set: function (value) {
this._thing = value;
},
enumerable: true,
configurable: true
});
I think that it can definitely start adding up over an entire project.
crisbeto
force-pushed
the
3225/overlay-setter-refactor
branch
2 times, most recently
from
65756d7 to
c690eb2
Compare
|
Addressed the feedback @jelbourn. |
…tialization * Refactors the `ConnectedOverlayDirective` not to depend on the order in which the `open` and `origin` properties are set. * Moves the `open`, `offsetX` and `offsetY` properties from using setters to `ngOnChanges` since the latter tends to output less JS. Fixes angular#3225.
crisbeto
force-pushed
the
3225/overlay-setter-refactor
branch
from
c690eb2 to
b2225e2
Compare
|
@jelbourn I've rebased this one and removed the changes to the |
| this.open ? this._attachOverlay() : this._detachOverlay(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you add a unit test for the case this fixes?
|
Added a test case for the changes @jelbourn. |
jelbourn
added
pr: lgtm
action: merge
|
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. |
4 participants
ConnectedOverlayDirectivenot to depend on the order in which theopenandoriginproperties are set.open,offsetXandoffsetYproperties from using setters tongOnChangessince the latter tends to output less JS.Fixes #3225.