fix(demo-app): fix aligned menu triggers and make page scrollable #8280
Conversation
googlebot
added
the
cla: yes
src/lib/menu/menu-trigger.ts
Outdated
| @@ -341,7 +343,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { | |||
| const posX: MenuPositionX = change.connectionPair.overlayX === 'start' ? 'after' : 'before'; | |||
| const posY: MenuPositionY = change.connectionPair.overlayY === 'top' ? 'below' : 'above'; | |||
|
|
|||
| this.menu.setPositionClasses(posX, posY); | |||
| // Note that the position changes may be outside the Angular zone | |||
| this._ngZone.run(() => this.menu.setPositionClasses(posX, posY)); | |||
There was a problem hiding this comment.
Does markForCheck in the ChangeDetectorRef not work for this situation?
There was a problem hiding this comment.
I thought detectChanges might work, but no.
The problem is that onPositionChange sometimes emits within and sometimes outside NgZone due to the scroll dispatcher. I don't have a complete grasp on it, and this might be better if onPositionChange always just emitted from within the zone instead of trying to bring it back in at the component level.
@crisbeto can you weigh in?
There was a problem hiding this comment.
Ideally we should be consistent in terms of where the event is being emitted, however with the way things are set up at the moment (onPositionChange isn't really a "change" event but more of an "update" event since it emits even if the position didn't change) we'd end up hitting the zone for all scroll events. We can address it better once the new connected positioning is in.
There was a problem hiding this comment.
It would be better at least to only enter the zone when posX or posY change
There was a problem hiding this comment.
Code-wise it looks good, however I think that it could lead to some performance issues since the onPositionChange currently emits whenever the position is updated, even if it didn't change, which will cause us to hit the zone for every scroll event. That being said, the issue is in the ConnectedPositionStrategy, not in the code from this PR. It may be better to hold off until we make the event a little smarter.
src/lib/menu/menu-trigger.ts
Outdated
| @@ -341,7 +343,8 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy { | |||
| const posX: MenuPositionX = change.connectionPair.overlayX === 'start' ? 'after' : 'before'; | |||
| const posY: MenuPositionY = change.connectionPair.overlayY === 'top' ? 'below' : 'above'; | |||
|
|
|||
| this.menu.setPositionClasses(posX, posY); | |||
| // Note that the position changes may be outside the Angular zone | |||
| this._ngZone.run(() => this.menu.setPositionClasses(posX, posY)); | |||
There was a problem hiding this comment.
Ideally we should be consistent in terms of where the event is being emitted, however with the way things are set up at the moment (onPositionChange isn't really a "change" event but more of an "update" event since it emits even if the position didn't change) we'd end up hitting the zone for all scroll events. We can address it better once the new connected positioning is in.
|
@crisbeto that sounds good - I didn't even notice it was emitting on every event. I'm happy to repurpose this to be for just updating the demo app. Do you or @jelbourn have a design doc/canonical issue for tracking improvements to the strategy? I'd hate for some of these things to be forgotten once it is available. |
|
The new connected position strategy is tracked in #6534 |
willshowell
force-pushed
the
fix/menu-class-updates
branch
from
d8a9818 to
c6f1128
Compare
willshowell
changed the title
willshowell
changed the title
|
This now only updates the menu demo flex styles and adds extra height to demo scrolling behavior. |
crisbeto
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. |
This change keeps the classlist up to date when the menu panel is repositioned due to scrolling.
Since the menu does not need a correct
transform-originduring its exit animation, this is not really noticeable or necessary. However, some users may be looking for positioning classes, or the animation may change in the future.It also adds a scrolling to the demo app and fixes a misused style.