fix(menu): update position on every open #8653
Conversation
EladBezalel
commented
Nov 26, 2017
EladBezalel
commented
- menu overlay is only created once and the position of the overlay wasn't calculated again
googlebot
added
the
cla: yes
There was a problem hiding this comment.
LGTM, but I'm not sure how this plays in with #7376. AFAIK the position strategies aren't really intended to be overwritten after init. It would be better if we allowed for the position strategy's positions to be updated instead. We have that in the new connected positioning, but I'm not sure whether we should backport it to the current one. cc @jelbourn
src/lib/menu/menu-trigger.ts
Outdated
| @@ -313,6 +313,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { | |||
| const config = this._getOverlayConfig(); | |||
| this._subscribeToPositions(config.positionStrategy as ConnectedPositionStrategy); | |||
| this._overlayRef = this._overlay.create(config); | |||
| } else { | |||
There was a problem hiding this comment.
Flip the if/else here so it's something like this. It should be a little easier to read through:
if (this._overlayRef) {
// update position
} else {
// create overlay
}
- menu overlay is only created once and the position of the overlay wasn't calculated again
EladBezalel
force-pushed
the
fix/menu-position
branch
from
d7785cd
to
5e12f9a
Compare
|
This will indeed conflict with #7376, which I plan on getting in first |
… init Backports the ability to update a connected overlay's positioning from the new flexible connected position strategy. This helps facilitate cases like angular#8653 without having to overwrite the entire position strategy.
|
Thinking about this one again, I think that overwriting the position strategy can cause some subtle bugs, because it means that any subscriptions to the |
… init (angular#8800) Backports the ability to update a connected overlay's positioning from the new flexible connected position strategy. This helps facilitate cases like angular#8653 without having to overwrite the entire position strategy.
|
Closing this since (based on the comments) we'd want to do a different approach. |
|
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. |
