-
Notifications
You must be signed in to change notification settings - Fork 6.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(drawer): allow for drawer container to auto-resize while open #8488
Conversation
src/lib/sidenav/drawer.ts
Outdated
private _margins = {left: 0, right: 0}; | ||
|
||
/** Used for debouncing reflows. */ | ||
private _debouncer = new Subject<void>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call it _doCheckSubject
?
src/lib/sidenav/drawer.ts
Outdated
@@ -462,9 +490,15 @@ export class MatDrawerContainer implements AfterContentInit, OnDestroy { | |||
|
|||
this._changeDetectorRef.markForCheck(); | |||
}); | |||
|
|||
this._debouncer.pipe( | |||
debounceTime(10), // Arbitrary debounce time, less than a frame at 60fps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is debounce
what we want, doesn't audit
make more sense here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say so. audit
just throttles the events, but what we want is to only measure once if multiple events happen very close to each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the drawer were animating open, wouldn't it just keep denouncing until the animation is over and then jump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK animations don't trigger change detection. ngDoCheck
runs for every change detection cycle.
src/lib/sidenav/drawer.ts
Outdated
constructor(@Optional() private _dir: Directionality, private _element: ElementRef, | ||
private _renderer: Renderer2, private _ngZone: NgZone, | ||
constructor(@Optional() private _dir: Directionality, | ||
@Inject(MAT_DRAWER_DEFAULT_AUTOSIZE) defaultAutosize: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we move this to the end of the parameter list and make it optional it won't be breaking for users who are extending this class
src/lib/sidenav/sidenav.md
Outdated
issues. | ||
|
||
```html | ||
<mat-sidenav-container [autosize]="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this an embedded example? (#7775 will make the rest of these embedded examples as well).
Maybe also add an example to the demo app so we can manually test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what an embedded example would look like in this case, aside from a sidenav that resizes in a setTimeout
, but that's not very representative of a real use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have an icon drawer that expands to show full labels when the user hovers or clicks a button.
Or just have a button that toggles itself between shorter and longer width when clicked, not a practical use case but demonstrates how to use the feature and lets us manually test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've added a live example.
8da8c0b
to
1c685f4
Compare
Addressed the code-related feedback @mmalerba. |
1c685f4
to
8947aec
Compare
@@ -0,0 +1,16 @@ | |||
<mat-sidenav-container class="example-container" autosize> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change these to the drawer variants? I only want people to use sidenav for fullscreen layouts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just 1 nit. Feel free to add merge-ready after
8947aec
to
08fa7d0
Compare
@crisbeto please rebase. Thanks! |
08fa7d0
to
61783b6
Compare
Rebased @tinayuangao. |
61783b6
to
1436eee
Compare
Adds the `autosize` input that allows users to opt-in to drawers that auto-resize, similarly to the behavior before angular#6189. The behavior is off by default, because it's unnecessary for most use cases and can cause performance issues. Fixes angular#6743.
1436eee
to
ae04fcf
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. |
Adds the
autosize
input that allows users to opt-in to drawers that auto-resize, similarly to the behavior before #6189. The behavior is off by default, because it's unnecessary for most use cases and can cause performance issues.Fixes #6743.