feat(dialog): add dialog content elements #2090

crisbeto · 2016-12-06T19:19:04Z

Adds the following dialog-specific directives:

md-dialog-close - Closes the current dialog.
md-dialog-title - Title of a dialog.
md-dialog-content - Scrollable content for a dialog.
md-dialog-actions - Container for the bottom buttons in a dialog.

Fixes #1624.
Fixes #2042.

pietie · 2016-12-07T05:34:24Z

Out of interest, why is the md-dialog-title selector just an attribute?
If I compare it with md-card I would have expected it to be <md-dialog-title>

crisbeto · 2016-12-07T08:06:12Z

It's there to encourage you to use the appropriate native elements @pietie (e.g. h1, h2 etc), although I should've probably scoped it to those nodes. I'll add it later today. As for the card, I'm not sure what the reasoning is, but I suppose it's because the heading elements aren't always appropriate.

clydin · 2016-12-07T21:01:40Z

Have you considered an "align" attribute on md-dialog-actions similar to md-card? "center" would also be a nice addition to start|end as well.

crisbeto · 2016-12-07T21:09:45Z

Nope, I followed the example from #1624, but it sounds reasonable.

jelbourn

I pulled it down and this looks like just what I had in mind. Thanks!

I like the idea of adding align to md-dialog-actions in a follow-up PR.

jelbourn · 2016-12-07T23:29:56Z

src/lib/dialog/dialog-directives.ts

+@Directive({
+  selector: '[md-dialog-title]',
+  host: {
+    role: 'heading'


I don't think we should put a role on this- the user should just put it on the appropriate element (h2, header, etc.)

I added it in case the attribute was added to something like a div. I'll scope the selector to the proper nodes.

jelbourn · 2016-12-07T23:32:22Z

src/lib/dialog/dialog-directives.ts

+@Directive({
+  selector: '[md-dialog-content], md-dialog-content',
+  host: {
+    role: 'main'


I don't think we should have this role; nothing in the W3C authoring practices mentiones it and the docs for role="main" says there should only be one per "document" (for which a modal dialog doesn't count).

Shouldn't the content have some kind of role? The main was the only one I could find that seemed to fit.

I don't think it needs a role at all

jelbourn · 2016-12-07T23:34:30Z

src/lib/dialog/dialog.scss

+}
+
+[md-dialog-title] {
+  font-size: rem(2);


I'd like to avoid using this rem function for now... the fact that it's even here is a leftover from an earlier version. During beta we're going to work on a more comprehensive typography solution.

What should I go with in this case? Put in the computed value?

Yeah, go ahead with the computed value for now.

jelbourn · 2016-12-07T23:35:09Z

src/lib/dialog/dialog.scss

+  display: block;
+
+  &:last-child {
+    margin-bottom: -$md-dialog-padding;


Add a comment explaining what this style is for?

jelbourn · 2016-12-07T23:49:11Z

src/lib/dialog/dialog-directives.ts

+ * Button that will close the current dialog.
+ */
+@Directive({
+  selector: 'button[md-dialog-close]',


Also add mat- prefixed selectors (will be used for material1 compatibility mode).

jelbourn · 2016-12-07T23:49:48Z

src/lib/dialog/dialog.scss

+$md-dialog-padding: 24px !default;
+$md-dialog-border-radius: 2px !default;
+$md-dialog-max-width: 80vw !default;
+$md-dialog-max-height: 65vh !default;


Where do these max-height and max-width come from?

The max-width is the same that Material 1 used, the max-height is arbitrary. M1 has 80% for the max-height as well, although that's for the entire dialog, whereas here we only have it on the md-dialog-content.

Do we need to set a max-width and max-height at all, then? I could imagine some people wanting to do a full-screen dialog.

I'd say so, otherwise the dialog content won't become scrollable. Also I'm not sure whether fullscreen dialogs are in the spec.

actually yes, there are fullscreen dialogs in material guidelines https://material.io/guidelines/components/dialogs.html#dialogs-full-screen-dialogs

jelbourn · 2016-12-07T23:50:40Z

src/lib/dialog/dialog.scss

+[md-dialog-title] {
+  font-size: rem(2);
+  font-weight: bold;
+  letter-spacing: 0.005em;


Why is letter-spacing necessary here?

I went with whatever M1 was doing, I'm not sure what the reasoning for it was.

Let's remove it, then.

jelbourn · 2016-12-07T23:53:32Z

src/lib/dialog/dialog-directives.ts

+  selector: 'button[md-dialog-close]',
+  host: {
+    '(click)': 'dialog.closeTop()'
+  }


We need to add a default aria-label to this that says "Close dialog". Also a unit test that ensures an aria-label set by the user overrides the default.

jelbourn · 2016-12-07T23:54:55Z

src/lib/dialog/dialog-directives.ts

@@ -0,0 +1,49 @@
+import {Directive} from '@angular/core';


Let's call this file dialog-content-directives.ts

jelbourn · 2016-12-07T23:59:40Z

src/lib/dialog/dialog-directives.ts

+@Directive({
+  selector: 'button[md-dialog-close]',
+  host: {
+    '(click)': 'dialog.closeTop()'


I don't think this will work for situations where there are two dialogs open side-by-side instead of one on top of the other. I took a poke at the code and found that you can inject the MdDialogRef here because the content is loaded with an instance of DialogInjector, so this works:

/** * Button that will close the current dialog. */ @Directive({ selector: 'button[md-dialog-close]', host: { '(click)': 'dialogRef.close()' } }) export class MdDialogClose { constructor(public dialogRef: MdDialogRef<any>) { } }

With this change, I don't think we need closeTop any more.

Was going to say pretty much the same thing. As is it will break some more exotic use cases.

I went this route since it's not currently possible to have multiple dialogs side-by-side. Also I wasn't aware of the DialogInjector. I'll try it out.

crisbeto · 2016-12-08T18:43:05Z

Addressed the feedback @jelbourn.

jelbourn · 2016-12-14T03:44:55Z

LGTM, just needs rebase. Go ahead and apply the merge_ready label when it's rebased

Adds the following dialog-specific directives: * `md-dialog-close` - Closes the current dialog. * `md-dialog-title` - Title of a dialog. * `md-dialog-content` - Scrollable content for a dialog. * `md-dialog-actions` - Container for the bottom buttons in a dialog. Fixes angular#1624. Fixes angular#2042.

…ons.

chpasha · 2016-12-23T12:31:43Z

is it possible to override md-dialog-max-width somehow? I'm unable to change it to anything else in spite of !default attribute. The space is precious especially in xs-sm, hard coded 80vw seems to be too small

chpasha · 2016-12-23T12:35:33Z

one more thing - material design guidelines for dialogs (except for alertdialog) have an toolbar at the top, did you consider the possibility to give an option of aligning md-dialog-actions to the top or introducing one more directive like md-dialog-toolbar? It would be also awesome if that toolbar could have home button, title and action areas

crisbeto · 2016-12-23T12:47:29Z

@chpasha

How are you trying to override the max width? If overriding the SASS variable doesn't work, you should still be able to set this somewhere in your stylesheet:

md-dialog-container {
  max-width: <some value>;
}

The point of this PR was to get some of the dialog content elements as a base. We'll probably keep improving on them in the future.

chpasha · 2016-12-23T13:06:44Z

yes, selector in css works but I was hoping for more elegant solution via scss. Is there any point for defining those properties as !default since they cannot be overwritten in client library?
👍 fullscreen-style dialogs with toolbar at the top and basic elements like home button, title, content and action buttons would be cool

antonyRoberts · 2017-01-05T23:50:45Z

@crisbeto Where exactly might you override the max-width in the CSS? Setting it on the parent component calling the Dialog or the Dialog itself's style does not seem to change the max-width to whatever it is set to.
Fullscreen dialogues are important for mobile.

http://plnkr.co/edit/7CE3VG1LUj2VtLCtdaOM?p=preview

@chpasha Did you have any luck?

chpasha · 2017-01-06T11:25:42Z

yes, I did succeed http://plnkr.co/edit/B46euKJxtf8RXu1O0smZ?p=preview
two changes

encapsulation: ViewEncapsulation.None on Component
md-dialog-container (not .md-dialog-container) in styles - and don't forget that it wont grow bigger if you content is small - to see that it works, I added width: 100vw

angular-automatic-lock-bot · 2019-09-06T06:02:57Z

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

crisbeto added the pr: needs review label Dec 6, 2016

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 6, 2016

jelbourn reviewed Dec 8, 2016

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Dec 14, 2016

crisbeto added 9 commits December 14, 2016 17:40

Rename the dialog directives file.

becff1d

Add the selectors for Material 1 compatibility.

cf767fe

Remove the closeTop method and use the dialogRef instead.

e7712cb

Add an aria-label to the close button and simplify the testing setup.

4f078ec

Remove redundant element roles.

6e89905

Use the computed value on the dialog title font size.

1c88fbc

Remove the letter spacing override on the dialog title.

8043139

Add a comment regarding the negative bottom margin on the dialog acti…

eeff3e1

…ons.

crisbeto force-pushed the 1624/dialog-content-elements branch from 54ae810 to eeff3e1 Compare December 14, 2016 16:40

crisbeto added the action: merge The PR is ready for merge by the caretaker label Dec 14, 2016

jelbourn merged commit cac72aa into angular:master Dec 16, 2016

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

feat(dialog): add dialog content elements #2090

feat(dialog): add dialog content elements #2090

Conversation

crisbeto commented Dec 6, 2016

pietie commented Dec 7, 2016

crisbeto commented Dec 7, 2016 • edited Loading

clydin commented Dec 7, 2016

crisbeto commented Dec 7, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chpasha Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Dec 8, 2016

jelbourn commented Dec 14, 2016

chpasha commented Dec 23, 2016

chpasha commented Dec 23, 2016

crisbeto commented Dec 23, 2016

chpasha commented Dec 23, 2016

antonyRoberts commented Jan 5, 2017 • edited Loading

chpasha commented Jan 6, 2017

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

crisbeto commented Dec 7, 2016 •

edited

Loading

chpasha Dec 23, 2016 •

edited

Loading

antonyRoberts commented Jan 5, 2017 •

edited

Loading