Skip to content

refactor(dialog): better handling of scrollable content #2546

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

Closed
wants to merge 2 commits into from
Closed

refactor(dialog): better handling of scrollable content #2546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55f1974
refactor(dialog): better handling of scrollable content
crisbeto Dec 31, 2016
02679dc
Merge branch 'master' into 2481/dialog-scrolling
crisbeto Feb 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/lib/dialog/dialog-container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
import {BasePortalHost, ComponentPortal, PortalHostDirective, TemplatePortal} from '../core';
import {MdDialogConfig} from './dialog-config';
import {MdDialogRef} from './dialog-ref';
import {MD_DIALOG_CONTENT_SELECTOR} from './dialog-content-directives';
import {MdDialogContentAlreadyAttachedError} from './dialog-errors';
import {FocusTrap} from '../core/a11y/focus-trap';
import 'rxjs/add/operator/first';
Expand Down Expand Up @@ -61,6 +62,16 @@ export class MdDialogContainer extends BasePortalHost implements OnDestroy {
}

let attachResult = this._portalHost.attachComponentPortal(portal);
let componentElement = attachResult.location.nativeElement;

// Add a class that we can use for styling the root element.
this._renderer.setElementClass(componentElement, 'md-dialog-root', true);

// Add flexbox styling if the user is using the `md-dialog-content`.
if ('querySelector' in componentElement) {
this._renderer.setElementClass(componentElement, 'md-dialog-root-flex',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should do this. This will set display: flex on the host element of the loaded component, which will potentially change the layout of the contents of that element in a way that the user did not intend; we should generally avoid setting any styles to elements Material doesn't own.

Why not add an element to the dialog-container template for this (and for md-dialog-root)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning with using flex is that if the user has their content in a md-dialog-content, then it's safe to add flexbox, because they, most likely, don't have any other content at the root (apart from md-dialog-title and md-dialog-actions).

As for adding an extra element, I'm not sure I get what you mean. Isn't this what md-dialog-container already does?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming back to this PR: which element exactly is this class applied to?

Copy link
Member Author

@crisbeto crisbeto Mar 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've keeping this around for tracking. I was been working on a different PR that does the something similar, but with a nicer approach. I haven't been able to get it to work properly cross-browser yet. We can close this if necessary.

!!componentElement.querySelector(MD_DIALOG_CONTENT_SELECTOR));
}

// If were to attempt to focus immediately, then the content of the dialog would not yet be
// ready in instances where change detection has to run first. To deal with this, we simply
Expand Down
5 changes: 4 additions & 1 deletion src/lib/dialog/dialog-content-directives.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import {Directive, Input} from '@angular/core';
import {MdDialogRef} from './dialog-ref';

export const MD_DIALOG_CONTENT_SELECTOR = '[md-dialog-content], md-dialog-content' +
', [mat-dialog-content], mat-dialog-content';


/**
* Button that will close the current dialog.
Expand Down Expand Up @@ -33,7 +36,7 @@ export class MdDialogTitle { }
* Scrollable content container of a dialog.
*/
@Directive({
selector: '[md-dialog-content], md-dialog-content, [mat-dialog-content], mat-dialog-content'
selector: MD_DIALOG_CONTENT_SELECTOR
})
export class MdDialogContent { }

Expand Down
14 changes: 11 additions & 3 deletions src/lib/dialog/dialog.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
$md-dialog-padding: 24px !default;
$md-dialog-border-radius: 2px !default;
$md-dialog-max-width: 80vw !default;
$md-dialog-max-height: 65vh !default;
$md-dialog-max-height: 80vh !default;

md-dialog-container {
@include md-elevation(24);
Expand All @@ -15,7 +15,6 @@ md-dialog-container {
border-radius: $md-dialog-border-radius;
box-sizing: border-box;
overflow: auto;
max-width: $md-dialog-max-width;

// The dialog container should completely fill its parent overlay element.
width: 100%;
Expand All @@ -26,11 +25,20 @@ md-dialog-container {
}
}

.md-dialog-root {
max-height: calc(#{$md-dialog-max-height} - #{$md-dialog-padding * 2});
max-width: $md-dialog-max-width;

&.md-dialog-root-flex {
display: flex;
flex-flow: column;
}
}

md-dialog-content, [md-dialog-content], mat-dialog-content, [mat-dialog-content] {
display: block;
margin: 0 $md-dialog-padding * -1;
padding: 0 $md-dialog-padding;
max-height: $md-dialog-max-height;
overflow: auto;
}

Expand Down
25 changes: 23 additions & 2 deletions src/lib/dialog/dialog.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,23 @@ describe('MdDialog', () => {
expect(overlayContainerElement.querySelectorAll('md-dialog-container').length).toBe(0);
});

it('should add a class for styling to the root element', () => {
dialog.open(PizzaMsg);

let pizzaMsgContainer = overlayContainerElement.querySelector('pizza-msg');

expect(pizzaMsgContainer.classList).toContain('md-dialog-root');
expect(pizzaMsgContainer.classList).not.toContain('md-dialog-root-flex');
});

it('should add an extra flexbox class to components that use md-dialog-content', () => {
dialog.open(ContentElementDialog);

let dialogContainer = overlayContainerElement.querySelector('content-element-dialog');

expect(dialogContainer.classList).toContain('md-dialog-root-flex');
});

describe('disableClose option', () => {
it('should prevent closing via clicks on the backdrop', () => {
dialog.open(PizzaMsg, {
Expand Down Expand Up @@ -477,7 +494,10 @@ class ComponentWithChildViewContainer {
}

/** Simple component for testing ComponentPortal. */
@Component({template: '<p>Pizza</p> <input> <button>Close</button>'})
@Component({
template: '<p>Pizza</p> <input> <button>Close</button>',
selector: 'pizza-msg'
})
class PizzaMsg {
constructor(public dialogRef: MdDialogRef<PizzaMsg>,
public dialogInjector: Injector) {}
Expand All @@ -491,7 +511,8 @@ class PizzaMsg {
<button md-dialog-close [aria-label]="closeButtonAriaLabel">Close</button>
<div md-dialog-close>Should not close</div>
</md-dialog-actions>
`
`,
selector: 'content-element-dialog'
})
class ContentElementDialog {
closeButtonAriaLabel: string;
Expand Down