docs(demos): add dialog demos #5666

amcdnl · 2017-07-11T00:03:41Z

Adds demos for:

Basic Dialog
Dialog Results
Dialog Content
Complex Dialog Overview
Enhanced docs for data purpose to be more clear when passing data

amcdnl · 2017-07-11T00:06:28Z

When merged, will need to ensure this is merged at same time - angular/material.angular.io#216

jelbourn · 2017-07-11T18:28:52Z

@crisbeto should also take a look

crisbeto

A few nitpicks.

crisbeto · 2017-07-11T18:30:56Z

src/material-examples/dialog-basic/dialog-basic-example.ts

+  templateUrl: 'dialog-basic-example.html',
+})
+export class DialogBasicExample {
+  constructor(public dialog: MdDialog) {}


This doesn't need to be public.

crisbeto · 2017-07-11T18:33:35Z

src/material-examples/dialog-content/dialog-content-example-dialog.html

+<md-dialog-content>Are you sure?</md-dialog-content>
+<md-dialog-actions>
+  <button md-button tabindex="-1">Not Tabbable</button>
+  <button md-button [md-dialog-close]="true" tabindex="1">Yes</button>


A more realistic example for this case would be to have an input and have the md-dialog-close pass the input value back to the afterClosed.

@crisbeto - I have this demo in the overview, do you think we need to do it here too?

Sorry, I must've missed the one below.

crisbeto · 2017-07-11T18:34:50Z

src/material-examples/dialog-overview/dialog-overview-example-dialog.html

-<p> Hi, I'm a dialog! </p>
+<h1 md-dialog-title>Hi {{data.name}}</h1>
+<div md-dialog-content>
+  <p>Whats your favorite animal?</p>


Whats -> What's.

crisbeto · 2017-07-11T18:35:27Z

src/material-examples/dialog-overview/dialog-overview-example.html

+<ol>
+  <li>
+    <md-input-container>
+      <input mdInput [(ngModel)]="name" placeholder="Whats your name?">


Whats -> What's.

crisbeto · 2017-07-11T18:36:42Z

src/material-examples/dialog-overview/dialog-overview-example.html

+  <li *ngIf="animal">
+    You chose: <i>{{animal}}</i>
+  </li>
+</ol>


Missing trailing newline at the end of the file.

Odd that linting isn't picking this up.

crisbeto · 2017-07-11T18:38:29Z

src/material-examples/material-module.ts

@@ -40,7 +40,8 @@ import {
    MdToolbarModule,
    MdTooltipModule,
    MdPaginatorModule,
-    MdSortModule
+    MdSortModule,
+    MdTableModule


Looks like this adds MdTableModule for a second time (it's already defined on line 39).

crisbeto

LGTM

jelbourn · 2017-07-12T20:49:36Z

src/material-examples/dialog-data/dialog-data-example.ts

+  openDialog() {
+    this.dialog.open(DialogDataExampleDialog, {
+      data: {
+        animal: 'panda'


This data should come from an <input> next to the dialog open button, to demonstrate the value coming from the user.

Per our conversation, we discussed using the example of the overview to demonstrate this and keeping this demo basic.

jelbourn · 2017-07-12T20:51:55Z

src/material-examples/dialog-content/dialog-content-example.ts

+/**
+ * @title Dialog Content
+ */
+@Component({


"Dialog content" by itself doesn't really communicate what this example is for. It should be something like
"Dialog with header, scrollable content, and actions". The content should be long enough to cause scrolling and wide enough that the actions are aligned to either the right or left of the dialog.

I addressed the scrolling and title, can you expand on what you had in mind for the button alignment?

jelbourn · 2017-07-12T20:52:42Z

src/material-examples/dialog-data/dialog-data-example.ts

+import {MdDialog, MD_DIALOG_DATA} from '@angular/material';
+
+/**
+ * @title Dialog Data Sharing


I'd call this "Injecting data when opening a dialog"

jelbourn · 2017-07-12T20:56:57Z

src/lib/dialog/dialog.md

@@ -1,7 +1,7 @@
 The `MdDialog` service can be used to open modal dialogs with Material Design styling and 
 animations.

-<!-- example(dialog-overview) -->
+<!-- example(dialog-basic) -->


Is this supposed to be dialog-overview?

I think we should probably get rid of this "dialog-basic" example because it is too basic; it doesn't really show anything. The first example really needs to showcase the component.

Done. Moved the first demo to the complex overview example.

jelbourn · 2017-07-12T20:58:41Z

Also, nothing really highlights returning data from the dialog like we have now. I think this should be featured more predominantly.

In removing dialog-result as an example, you would also have to remove reference to it from documentation-items.ts (it's 404-ing now).

amcdnl · 2017-07-12T21:16:11Z

@jelbourn - Already done, see comment above referencing the PR - angular/material.angular.io#216

… scrollable

jelbourn

LGTM

jelbourn · 2017-07-13T17:37:14Z

@amcdnl lint error

ERROR: /home/travis/build/angular/material2/src/material-examples/dialog-content/dialog-content-example.ts[18, 1]: trailing whitespace

elvirdolic · 2017-07-14T06:19:18Z

It would be great to share an example with async data and how to share data.

jelbourn · 2017-07-14T21:16:57Z

@amcdnl you can add the "merge_ready" label back after fixing the lint warning

angular-automatic-lock-bot · 2019-09-06T17:03:03Z

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

docs(demos): add dialog demos

b78b5d1

amcdnl requested a review from jelbourn July 11, 2017 00:03

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 11, 2017

Merge branch 'master' into dialog-demos

31306d5

amcdnl mentioned this pull request Jul 11, 2017

docs(demos): updates dialog example to overivew angular/material.angular.io#216

Merged

amcdnl self-assigned this Jul 11, 2017

chore(lint): fix lint errors

bcd3d7b

jelbourn requested a review from crisbeto July 11, 2017 18:28

crisbeto requested changes Jul 11, 2017

View reviewed changes

demos(demos): address PR feedback

fba0f16

crisbeto approved these changes Jul 12, 2017

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 12, 2017

jelbourn removed the action: merge The PR is ready for merge by the caretaker label Jul 12, 2017

jelbourn reviewed Jul 12, 2017

View reviewed changes

docs(demos): remove basic demos, rename demos, update content demo to…

c236619

… scrollable

jelbourn approved these changes Jul 12, 2017

View reviewed changes

jelbourn added action: merge The PR is ready for merge by the caretaker pr: merge safe labels Jul 12, 2017

jelbourn removed the action: merge The PR is ready for merge by the caretaker label Jul 14, 2017

docs(demos): fix lint error

e24f244

amcdnl added the action: merge The PR is ready for merge by the caretaker label Jul 15, 2017

jelbourn merged commit 5962c93 into angular:master Jul 17, 2017

amcdnl deleted the dialog-demos branch July 20, 2017 14:46

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

docs(demos): add dialog demos #5666

docs(demos): add dialog demos #5666

Uh oh!

Conversation

amcdnl commented Jul 11, 2017

Uh oh!

amcdnl commented Jul 11, 2017

Uh oh!

jelbourn commented Jul 11, 2017

Uh oh!

crisbeto 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!

amcdnl Jul 11, 2017 • 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!

crisbeto 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jul 12, 2017

Uh oh!

amcdnl commented Jul 12, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jul 13, 2017

Uh oh!

elvirdolic commented Jul 14, 2017

Uh oh!

jelbourn commented Jul 14, 2017

Uh oh!

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

Uh oh!

Uh oh!

amcdnl Jul 11, 2017 •

edited

Loading