-
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
feat(snack-bar): set snack bar to be responsive #7485
Conversation
0023ed8
to
69cbfca
Compare
@@ -33,6 +33,12 @@ $mat-snack-bar-spacing-margin: 24px !default; | |||
transform: translateY(-100%); | |||
} | |||
|
|||
&.mobile { |
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.
A few notes:
.mobile
seems like something that could clash with user styles. What about calling itmat-snackbar-mobile
?- Renaming the class will let you avoid the nesting.
- Is there a particular reason for going with viewport units rather than percent? I have a vague memory about
100vw
causing content to go behind the scrollbar on Windows.
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.
Yep, definitely forgot to rename the class after I got it working.
Because the overlay container does not establish itself as the entire width of the viewport, width: 100%
turns into the width of the child based on content, so we use 100vw
.
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.
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.
The issue that we run into with changing the width of the overlay container, is that the snackbar will always stay in its most expanded state (as limited by max-width
)
I will have to continue to investigate
9e75237
to
3ff026c
Compare
@crisbeto Comments addresed. I moved the observation to Feel free to take a look when you have a chance. |
src/lib/snack-bar/snack-bar.ts
Outdated
@@ -145,6 +148,18 @@ export class MatSnackBar { | |||
// We can't pass this via the injector, because the injector is created earlier. | |||
snackBarRef.instance = contentRef.instance; | |||
|
|||
// Subscribe to the breakpoint observer and attacg the mat-snack-bar-handset class as |
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.
Small typo :)
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.
Mostly LGTM, some minor feedback.
src/lib/snack-bar/snack-bar.ts
Outdated
// appropriate. This class is applied to the overlay element because the overlay must expand to | ||
// fill the width of the screen for full width snackbars. | ||
const isHandset = this._breakpointObserver.observe(Breakpoints.Handset).subscribe(state => { | ||
overlayRef.overlayElement.classList.toggle('mat-snack-bar-handset', state.matches); |
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.
The second param for classList.toggle
doesn't work on IE11. This should use add
/remove
instead.
src/lib/snack-bar/snack-bar.ts
Outdated
}); | ||
// Unsubscribe from the isHandset subscription when the overlay is detached from the view. | ||
const overlayDetach = overlayRef.detachments().subscribe(() => { | ||
overlayDetach.unsubscribe(); |
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.
A cleaner approach could be to use takeUntil
. E.g.
this._breakpointObserver.observe(Breakpoints.Handset)
.takeUntil(overlayRef.detachments().first())
.subscribe(() => ...);
This way you don't have to keep track of subscriptions.
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.
Good call
3ff026c
to
0d02a73
Compare
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.
LGTM
src/lib/snack-bar/snack-bar.ts
Outdated
// appropriate. This class is applied to the overlay element because the overlay must expand to | ||
// fill the width of the screen for full width snackbars. | ||
RxChain.from(this._breakpointObserver.observe(Breakpoints.Handset)) | ||
.call(takeUntil, RxChain.from(overlayRef.detachments()).call(first).result()) |
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.
For a single operator you don't really need the RxChain
, you can just .call
it directly. E.g. first.call(overlayRef.detachments())
.
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 did find that frustrating to have it in RxChain
again, not sure why I didn't think to look at calling it this way.
0d02a73
to
a423bb0
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. |
Fixes #3939