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.

Sign up for GitHub

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

Jump to bottom

feat(md-snack-bar): Create initial MdSnackBar. #1296

Merged

kara merged 1 commit into angular:master from josephperrott:master

Sep 23, 2016

Member

josephperrott commented Sep 21, 2016

For #115

googlebot added cla: yes labels

josephperrott force-pushed the master branch 5 times, most recently from af6db23 to 7f1485a Compare

September 21, 2016 22:40

googlebot commented Sep 21, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment

googlebot commented Sep 21, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot added cla: no and removed cla: yes labels

josephperrott force-pushed the master branch from adf4e9f to 93db33b Compare

September 21, 2016 22:51

googlebot commented Sep 21, 2016

CLAs look good, thanks!

1 similar comment

googlebot commented Sep 21, 2016

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels

josephperrott force-pushed the master branch from 93db33b to 2978624 Compare

September 21, 2016 22:57

jelbourn added the pr: needs review label

jelbourn reviewed

View reviewed changes

Member

jelbourn left a comment

For unit tests, you should be able to do pretty much the same thing as dialog.

src/lib/snack-bar/simple-snack-bar.scss

+                    padding: 5px;
+                    text-transform: uppercase;
+                  }
+              }

Member

jelbourn Sep 21, 2016

Can you refactor this to be like:

md-simple-snackbar {
  display: flex;
  justify-content: space-between;
}

.md-simple-snackbar-message {
  ...
}

.md-simple-snackbar-action {
  ...
}

Member Author

josephperrott Sep 22, 2016

Done

src/demo-app/snack-bar/snack-bar-demo.html

+                </div>
+              </div>
+              <button md-raised-button (click)="open()">Open</button>

Member

jelbourn Sep 21, 2016

OPEN all caps

Member Author

josephperrott Sep 22, 2016

Done

src/demo-app/snack-bar/snack-bar-demo.html

+                <div>
+                  <md-checkbox [(ngModel)]="action">Show button</md-checkbox>
+                  <md-input type="text" class="button-label-input"
+                            placeholder="Button Label"

Member

jelbourn Sep 21, 2016

placeholder="Snackbar action label"

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/base-snack-bar.ts

		import {MdSnackBarRef} from './snack-bar-ref';


		export class BaseSnackBarContent<T> {

Member

jelbourn Sep 21, 2016

What's this base class for, since it's only ever inherited by from SimpleSnackbar?

Member Author

josephperrott Sep 21, 2016

Custom components used for snack bar content would also need to inherit from BaseSnackBarContent as it defines the MdSnackBarRef attributes on the component as well as a dismiss method to dismiss the snack bar.

Member

jelbourn Sep 22, 2016

I don't want to require that any component you open in the sidenav inherits / implements anything. Inheritance is especially problematic because your component might already inherit from something else.

All of the end-developer's interactions with the snackbar should be done through the SnackBarRef

Member Author

josephperrott Sep 22, 2016

Gotcha, I was wanting to make sure that the MdSnackBarRef was available to every component it opens, but that can be the responsibility of the end-dev

Member

jelbourn Sep 22, 2016

In the dialog, we just make the MDDialogRef available for injection into the component (see DialogInjector). You don't have to worry about this for now, though.

src/lib/snack-bar/package.json

@@ @@ -0,0 +1,29 @@ @@
+              {
+                "name": "@angular2-material/snack-bar",
+                "version": "2.0.0-alpha.8-1",

Member

jelbourn Sep 21, 2016

alpha.8-2 (here and below)

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/snack-bar.ts

+                 */
+                open(message: string, buttonLabel: string,
+                     config: MdSnackBarConfig): MdSnackBarRef<SimpleSnackBar> {
+                  let simpleSnackBar = this.openFromComponent(SimpleSnackBar, config);

Member

jelbourn Sep 21, 2016

simpleSnackbarRef

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/snack-bar.ts

+                /**
+                 * Creates and dispatches a snack bar.
+                 */
+                open(message: string, buttonLabel: string,

Member

jelbourn Sep 21, 2016

buttonLabel > actionLabel ?

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/snack-bar.ts

+                 * Creates and dispatches a snack bar.
+                 */
+                open(message: string, buttonLabel: string,
+                     config: MdSnackBarConfig): MdSnackBarRef<SimpleSnackBar> {

Member

jelbourn Sep 21, 2016

config should be optional

Member Author

josephperrott Sep 21, 2016

Because the config includes the viewContainerRef, it will always need to be included.

src/lib/snack-bar/snack-bar.ts

+                  state.positionStrategy = this._overlay.position().global()
+                      .fixed()
+                      .centerHorizontally()
+                      .bottom('0px');

Member

jelbourn Sep 21, 2016

I think you can just do '0'

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/snack-bar.ts

+                /**
+                 * Places a new component as the content of the snack bar container.
+                 */
+                private _fillSnackBarContainer<T>(component: ComponentType<T>,

Member

jelbourn Sep 21, 2016

_attachSnackbarContent?

Member Author

josephperrott Sep 22, 2016

Done

josephperrott force-pushed the master branch from 2978624 to 9a92003 Compare

September 22, 2016 00:00

jelbourn reviewed

View reviewed changes

src/lib/snack-bar/base-snack-bar.ts

		import {MdSnackBarRef} from './snack-bar-ref';


		export class BaseSnackBarContent<T> {

Member

jelbourn Sep 22, 2016

I don't want to require that any component you open in the sidenav inherits / implements anything. Inheritance is especially problematic because your component might already inherit from something else.

All of the end-developer's interactions with the snackbar should be done through the SnackBarRef

src/lib/snack-bar/package.json

+                "peerDependencies": {
+                  "@angular2-material/core": "2.0.0-alpha.8-2"
+                }
+              }

Member

jelbourn Sep 22, 2016

You can delete this file, we won't need it for alpha.9

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/simple-snack-bar.scss

		@@ -0,0 +1,28 @@
		md-simple-snackbar {
		display: flex;

Member

jelbourn Sep 22, 2016

Too much indent

Member Author

josephperrott Sep 22, 2016

Done

src/lib/snack-bar/snack-bar-config.ts

+              export class MdSnackBarConfig {
+                /** The aria-role of the snack bar. */
+                role: SnackBarRole = 'alert';

Member

jelbourn Sep 22, 2016

'polite' isn't a valid role value. This should be used as the politeness that's passed through to the MdLiveAnnouncer

googlebot commented Sep 22, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment

googlebot commented Sep 22, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot added cla: no and removed cla: yes labels

josephperrott force-pushed the master branch 3 times, most recently from a1d68c3 to ee191f9 Compare

September 22, 2016 16:26

jelbourn added the cla: yes label

josephperrott force-pushed the master branch from ee191f9 to 617f514 Compare

September 22, 2016 16:41

googlebot added cla: no and removed cla: yes labels

josephperrott force-pushed the master branch 5 times, most recently from e6cdd55 to 344b318 Compare

September 23, 2016 00:25

googlebot commented Sep 23, 2016

CLAs look good, thanks!

1 similar comment

googlebot commented Sep 23, 2016

CLAs look good, thanks!

googlebot added cla: yes and removed cla: no labels

jelbourn reviewed

View reviewed changes

src/lib/snack-bar/snack-bar-container.html

		@@ -0,0 +1 @@
		<template portalHost></template>

Member

jelbourn Sep 23, 2016

Add a wrapper div with the following:

<div aria-live="assertive" aria-atomic="true" role="alert">

Eventually this needs to use MdLiveAnnouncer, but this can suffice for now.

Member Author

josephperrott Sep 23, 2016

Added MdLiveAnnouncer, I have role="alert", but not the others currently.

josephperrott force-pushed the master branch 6 times, most recently from c1299ea to f715eba Compare

September 23, 2016 06:32

Member Author

josephperrott commented Sep 23, 2016

Tests added.
MdLiveAnnouncer is now used.

jelbourn reviewed

View reviewed changes

Member

jelbourn left a comment

LGTM aside from a last few nits

src/demo-app/snack-bar/snack-bar-demo.scss

		@@ -0,0 +1,3 @@
		.button-label-input {

Member

jelbourn Sep 23, 2016

Prefix this with demo-

Member Author

josephperrott Sep 23, 2016

Done.

src/lib/snack-bar/snack-bar.spec.ts


		let containerElement = overlayContainerElement.querySelector('snack-bar-container');

		expect(containerElement.getAttribute('role')).toBe('alert');

Member

jelbourn Sep 23, 2016

One new thing I want to start doing is adding a message to asserts. This one would be something like

expect(containerElement.getAttribute('role'))
    .toBe('alert', 'Expected snackbar container to have role="alert"');

I only found out you could do this last week, and it makes test failures much easier to quickly understand.

Member Author

josephperrott Sep 23, 2016

Done

src/lib/snack-bar/snack-bar.spec.ts

+                  snackBarRef.dismiss();
+                  expect(dismissed).toBeTruthy();
+                  expect(overlayContainerElement.innerHTML.length).toBe(0);

Member

jelbourn Sep 23, 2016

expect(overlayContainer.innerHTML).toBe('') ?

Member Author

josephperrott Sep 23, 2016

Switched over to checking child node count since that's what we are really trying to check.


          feat(md-snack-bar): Create initial MdSnackBar.

7151d93

josephperrott force-pushed the master branch from f715eba to 7151d93 Compare

September 23, 2016 18:25

Member

jelbourn commented Sep 23, 2016

LGTM

jelbourn added pr: lgtm action: merge and removed pr: needs review labels

kara merged commit a732e88 into angular:master

angular-automatic-lock-bot bot commented Sep 6, 2019

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

angular-automatic-lock-bot bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge cla: yes