Skip to content

Conversation

crisbeto
Copy link
Member

  • 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 #3225.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Feb 21, 2017
@kara
Copy link
Contributor

kara commented Mar 3, 2017

@crisbeto This appears to break md-select panel positioning. Can you fix?

@crisbeto
Copy link
Member Author

crisbeto commented Mar 4, 2017

Fixed @kara. I don't know if calling detectChanges is considered a code smell, but it should be shorter than getters/setters. If you think it may be problematic, I'll switch it back.

@@ -172,6 +144,20 @@ export class ConnectedOverlayDirective implements OnDestroy {
this._destroyOverlay();
}

ngOnChanges(changes: SimpleChanges) {
if (changes['open']) {
changes['open'].currentValue ? this._attachOverlay() : this._detachOverlay();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you check changes['open'], shouldn't you be able to just use this.open?
(same for others)

@@ -577,6 +577,7 @@ export class MdSelect implements AfterContentInit, ControlValueAccessor, OnDestr
}

this._checkOverlayWithinViewport(maxScroll);
this._changeDetectorRef.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment explaining why this detectChanges call is necessary

this._position.withOffsetX(offsetX);
}
}
@Input() offsetX: number = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How significant is the code size difference? We use quite a lot of getter/setters.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 crisbeto force-pushed the 3225/overlay-setter-refactor branch 2 times, most recently from 65756d7 to c690eb2 Compare March 25, 2017 11:00
@crisbeto
Copy link
Member Author

Addressed the feedback @jelbourn.

@kara kara removed their assignment Apr 20, 2017
crisbeto added 2 commits May 9, 2017 21:14
…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 crisbeto force-pushed the 3225/overlay-setter-refactor branch from c690eb2 to b2225e2 Compare May 9, 2017 19:14
@crisbeto
Copy link
Member Author

crisbeto commented May 9, 2017

@jelbourn I've rebased this one and removed the changes to the offsetX and offsetY since they also turned out to be a bit dangerous. Can you take another look?

@crisbeto crisbeto assigned jelbourn and unassigned crisbeto May 9, 2017
this.open ? this._attachOverlay() : this._detachOverlay();
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a unit test for the case this fixes?

@crisbeto
Copy link
Member Author

Added a test case for the changes @jelbourn.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 11, 2017
@kara kara merged commit 2c0506c into angular:master May 11, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectedOverlayDirective relies on binding order
4 participants