feat(material-experimental/snack-bar): add MDC-based snack-bar #19738

andrewseguin · 2020-06-23T22:57:20Z

No description provided.

jelbourn

Did you have to touch the tests much, or are they copied pretty much identically (barring CSS class changes)?

jelbourn · 2020-06-24T03:18:51Z

src/material-experimental/mdc-snack-bar/module.ts

+/** Directive that should be applied to the text element to be rendered in the snack bar. */
+@Directive({
+  selector: `[matSnackBarLabel]`,
+  host: {
+    'class': 'mat-mdc-snack-bar-label mdc-snackbar__label',
+  }
+})
+export class MatSnackBarLabel {}
+
+/** Directive that should be applied to the element containing the snack bar's action buttons. */
+@Directive({
+  selector: `[matSnackBarActions]`,
+  host: {
+    'class': 'mat-mdc-snack-bar-actions mdc-snackbar__actions',
+  }
+})
+export class MatSnackBarActions {}
+
+/** Directive that should be applied to each of the snack bar's action buttons. */
+@Directive({
+  selector: `[matSnackBarAction]`,
+  host: {
+    'class': 'mat-mdc-snack-bar-action mdc-snackbar__action',
+  }
+})
+export class MatSnackBarAction {}


Move these directive to their own file like snack-bar-content.ts?

(side note, I regret deeply that we have "snack-bar" instead of "snackbar")

jelbourn · 2020-06-24T03:23:24Z

src/material-experimental/mdc-snack-bar/simple-snack-bar.ts

+  encapsulation: ViewEncapsulation.None,
+  changeDetection: ChangeDetectionStrategy.OnPush,
+  host: {
+    '[class.mat-mdc-simple-snack-bar]': 'true',


I don't think this needs to be a binding unless I'm forgetting some edge case

jelbourn · 2020-06-24T03:26:41Z

src/material/snack-bar/simple-snack-bar.ts

+ * Internal interface for a simple snack bar component..
+ * @docs-private
+ */
+export interface MatSimpleSnackBarInterface {


We generally want to avoid suffixing with "Interface"

I'm at a loss for names. I'd like to call it SimpleSnackBar and let the implementations be MatSimpleSnackBar, but our legacy snack bar is already named SimpleSnackBar

BaseSimpleSnackBar?

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

crisbeto · 2020-06-24T06:52:42Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+  // tslint:disable-next-line:validate-decorators
+  changeDetection: ChangeDetectionStrategy.Default,
+  encapsulation: ViewEncapsulation.None,
+  animations: [],


Doesn't seem to be used.

crisbeto · 2020-06-24T06:54:38Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+    // specifically the MDC label. If not, the container should apply the MDC label class to this
+    // component's label container, which will apply MDC's label styles to the attached view.
+    const attachedViewHasMdcLabel =
+        !!this._label.nativeElement.querySelector('.mdc-snackbar__label');


This is being run on every change detection cycle so it's very performance sensitive. Can we avoid querying the DOM and deduce the class some other way?

This is my least favorite part of the implementation, but I'm not sure how to get around it. As far as I can tell, it'll be difficult for the container to know whether the user provided a label and action using the MDC classes (either directly or with our directives).

I'd like to do this once, but I suspect we'll see bugs where the user provides something wrapped in an ngIf or ng-template that is loaded dynamically. Perhaps there could be a compromise where we perform this once at initialization, and allow for an API that lets the user explicitly say that their content will dynamically have the MDC classes

Maybe we can match it with a directive and query it with a ContentChild?

That seems like it would work

The user-provided component or template doesn't get included as part of either the content or view of the snack bar container. Technically I think the portal could provide this info through its view container - is it possible to make the query on the container's behalf?

Ah, I see, I forgot it was in a component (my brain spaced on this being snackbar).

Since snackbar content is generally simple and because they're short-lived, this is probably fine.

crisbeto · 2020-06-24T06:54:56Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+    // component's label container, which will apply MDC's label styles to the attached view.
+    const attachedViewHasMdcLabel =
+        !!this._label.nativeElement.querySelector('.mdc-snackbar__label');
+    this._label.nativeElement.classList.toggle('mdc-snackbar__label', !attachedViewHasMdcLabel);


IE doesn't support the second argument for classList.toggle.

crisbeto · 2020-06-24T06:55:19Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+export class MatSnackBarContainer extends BasePortalOutlet
+    implements MatSnackBarContainerInterface, AfterViewChecked, OnDestroy {
+  /** Subject for notifying that the snack bar has exited from view. */
+  readonly _onExit: Subject<any> = new Subject();


Should we type these subjects as void?

crisbeto · 2020-06-24T06:56:10Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+    // Mark this element with an 'exit' attribute to indicate that the snackbar has
+    // been dismissed and will soon be removed from the DOM. This is used by the snackbar
+    // test harness.
+    this._elementRef.nativeElement.setAttribute('mat-exit', '');


You should be able to set this through the host binding.

crisbeto · 2020-06-24T06:56:34Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+    this._elementRef.nativeElement.setAttribute('mat-exit', '');
+
+    this._mdcFoundation.close();
+    return this._onExit;


Should this have an asObservable call so we don't expose the subject?

crisbeto · 2020-06-24T06:57:55Z

src/material-experimental/mdc-snack-bar/snack-bar-container.ts

+
+  /** Makes sure the exit callbacks have been invoked when the element is destroyed. */
+  ngOnDestroy() {
+    this._mdcFoundation.close();


Should we also destroy the foundation here? AFAIK we create a new container each time a snack bar is opened.

Added it to the adapter's notifyClosed method. Here in ngOnDestroy is too early for it to be destroyed

andrewseguin · 2020-06-25T15:58:52Z

@jelbourn Tests were copied/pasted over, with the removal of tests for the no-longer-needed _animationState

jelbourn · 2020-06-25T16:19:09Z

src/material/snack-bar/simple-snack-bar.ts

+ * Internal interface for a simple snack bar component..
+ * @docs-private
+ */
+export interface BaseSimpleSnackBar {


How about something like TextOnlySnackbar or MessageOnlySnackbar

Works for me, I think BasicSnackBar could also work, but it's docs-private anyways and I'm not too worried about it. Went with TextOnlySnackBar

jelbourn · 2020-06-25T22:31:11Z

tools/public_api_guard/material/snack-bar.d.ts

@@ -53,6 +56,16 @@ export declare class MatSnackBarContainer extends BasePortalOutlet implements On
    static ɵfac: i0.ɵɵFactoryDef<MatSnackBarContainer, never>;
 }

+export interface MatSnackBarContainerInterface {


I missed this interface earlier. Maybe just SnackbarContainer?

jelbourn · 2020-06-25T22:32:57Z

tools/public_api_guard/material/snack-bar.d.ts

@@ -92,3 +105,11 @@ export declare class SimpleSnackBar {
    static ɵcmp: i0.ɵɵComponentDefWithMeta<SimpleSnackBar, "simple-snack-bar", never, {}, {}, never, never>;
    static ɵfac: i0.ɵɵFactoryDef<SimpleSnackBar, never>;
 }
+
+export interface TextOnlySnackBar {


Thinking about this more, could we prefix the newly added interfaces with an underscore? Maybe worth discussing in the team meeting, but I want to start making it more clear that some symbols are internal aside from just not documenting them.

jelbourn

LGTm, you can add merge-ready when ready

* feat(material-experimental/snack-bar): add MDC-based snack-bar * review changes * review changes 2 * fix import * api golden * review changes * remove underscore prefix * golden

angular#19738)" This reverts commit a63bfc5. This PR was marked as merge-safe, but it actually changed the existing snackbar.

#19738)" (#19799) This reverts commit a63bfc5. This PR was marked as merge-safe, but it actually changed the existing snackbar.

angular-automatic-lock-bot · 2020-07-30T00:08:17Z

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.}

andrewseguin added P2 The issue is important to a large percentage of users, with a workaround merge safe target: patch This PR is targeted for the next patch release labels Jun 23, 2020

andrewseguin requested review from jelbourn, crisbeto, devversion and mmalerba June 23, 2020 22:57

andrewseguin requested a review from a team as a code owner June 23, 2020 22:57

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 23, 2020

andrewseguin force-pushed the mdc-snack-bar branch from f6f243e to 55e2802 Compare June 24, 2020 02:06

jelbourn reviewed Jun 24, 2020

View reviewed changes

crisbeto reviewed Jun 24, 2020

View reviewed changes

andrewseguin requested review from jelbourn and crisbeto June 25, 2020 15:59

jelbourn reviewed Jun 25, 2020

View reviewed changes

andrewseguin requested a review from jelbourn June 25, 2020 17:18

jelbourn reviewed Jun 25, 2020

View reviewed changes

jelbourn approved these changes Jun 25, 2020

View reviewed changes

jelbourn added the lgtm label Jun 25, 2020

andrewseguin added 5 commits June 25, 2020 20:45

feat(material-experimental/snack-bar): add MDC-based snack-bar

31d5b7d

review changes

c36ff5f

review changes 2

bf8c3f4

fix import

47aaef5

api golden

ab0dbe5

andrewseguin force-pushed the mdc-snack-bar branch 2 times, most recently from a1e3be5 to f53b10f Compare June 26, 2020 14:56

review changes

60350a8

andrewseguin force-pushed the mdc-snack-bar branch from f53b10f to 60350a8 Compare June 26, 2020 14:59

remove underscore prefix

fb6d584

golden

334a39b

andrewseguin added the action: merge The PR is ready for merge by the caretaker label Jun 26, 2020

opozo mentioned this pull request Jun 27, 2020

feat(material-experimental/mdc-snackbar): Copy in the canonical MDC DOM and styles #19782

Closed

jelbourn merged commit a63bfc5 into angular:master Jun 29, 2020

jelbourn mentioned this pull request Jun 29, 2020

Revert "feat(material-experimental/snack-bar): add MDC-based snack-ba… #19799

Merged

jelbourn added a commit that referenced this pull request Jun 29, 2020

Revert "feat(material-experimental/snack-bar): add MDC-based snack-bar (

3a7e93c

#19738)" (#19799) This reverts commit a63bfc5. This PR was marked as merge-safe, but it actually changed the existing snackbar.

jelbourn added a commit that referenced this pull request Jun 29, 2020

Revert "feat(material-experimental/snack-bar): add MDC-based snack-bar (

ad23139

#19738)" (#19799) This reverts commit a63bfc5. This PR was marked as merge-safe, but it actually changed the existing snackbar.

andrewseguin mentioned this pull request Jun 29, 2020

feat(material-experimental/snack-bar): add MDC-based snack-bar #19800

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 30, 2020

feat(material-experimental/snack-bar): add MDC-based snack-bar #19738

feat(material-experimental/snack-bar): add MDC-based snack-bar #19738

Uh oh!

Conversation

andrewseguin commented Jun 23, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewseguin commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Jul 30, 2020

Uh oh!

Uh oh!

crisbeto Jun 24, 2020 •

edited

Loading