fix(dialog): improved handling of scrollable content #9236

crisbeto · 2018-01-04T19:04:53Z

Improves the handling of scrollable content inside the mat-dialog-content by using flexbox, rather than a hardcoded max-height, to define the content height. This resolves various issues where the dialog would go out of the screen at certain screen sizes or have multiple scrollbars.
Uses flexbox to ensure that the dialog content elements are always at the appropriate size.

Fixes #2481.
Fixes #3239.
Fixes #6584.
Fixes #8493.

jelbourn · 2018-01-11T21:19:27Z

src/lib/dialog/dialog-container.ts

@@ -120,7 +120,13 @@ export class MatDialogContainer extends BasePortalOutlet {
    }

    this._savePreviouslyFocusedElement();
-    return this._portalOutlet.attachComponentPortal(portal);
+
+    const componentRef = this._portalOutlet.attachComponentPortal(portal);


Add a comment here that explains why this extra class is added?

jelbourn · 2018-01-11T21:24:55Z

src/lib/dialog/dialog-container.html

@@ -1 +1,3 @@
-<ng-template cdkPortalOutlet></ng-template>
+<div class="mat-dialog-component-host">


Is this class meant to be both in the template and added manually?

Yeah, we need to add it through both to cover both template and dialog portals. The class is used to propagate the max-height down correctly since we've got a lot of nested divs.

jelbourn · 2018-01-11T21:29:11Z

src/lib/dialog/dialog-config.ts

@@ -74,7 +74,7 @@ export class MatDialogConfig<D = any> {
  maxWidth?: number | string = '80vw';

  /** Max-height of the dialog. If a number is provided, pixel units are assumed. */
-  maxHeight?: number | string;
+  maxHeight?: number | string = '80vh';


Was this 65vh before? If so, we probably would need to keep the same max-height until a major version change

It wasn't, it was the scrollable content that was 65vh, this applies the height to the overlay pane itself.

jelbourn · 2018-01-11T21:30:44Z

src/lib/dialog/dialog.scss

+  justify-content: center;
+  width: 100%;
+  flex-shrink: 0;
+  order: 1;


Wouldn't specifying the order take away the ability to put custom content in between the regions?

I suppose that it would, but the assumption is that if they're using the content element, they're putting all their content into it. I'm fine with removing it though, we haven't had it until now.

If the only reason would be to let people put the dialog directives in any order, I'd rather omit it to give people control in case they need to do something special

crisbeto · 2018-01-12T18:23:22Z

Addressed the feedback @jelbourn.

jelbourn

LGTM

dinamix1 · 2018-03-21T16:44:23Z

Looking forward to this merge!

pharv · 2018-04-23T23:59:37Z

Great work on this PR! I am really looking forward to this enhancement as well. It looks like the only reason it hasn't been merged in yet is because the Travis CI build failed due to server issues. Can someone please re-run the build? Thanks!

Devvox93 · 2018-06-22T12:41:53Z

What are the presubmit failures that are keeping this from being merged?
I, too, am having the same scrolling issues with the material dialogs.

ngbot · 2018-07-11T15:24:25Z

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

tammysock · 2018-07-17T18:09:50Z

This fix will be very helpful. Any updates on the PR?

swftvsn · 2018-08-23T07:49:49Z

Hi, I would really appreciate this fix too, we've been waiting for a year now for this to land (we're eager to take out our own hacks to overcome some issues that this would fix..)

So, any news about this @crisbeto @jelbourn ?

jpike88 · 2019-03-05T06:11:11Z

@crisbeto is this PR going to happen?

* Improves the handling of scrollable content inside the `mat-dialog-content` by using flexbox, rather than a hardcoded `max-height`, to define the content height. This resolves various issues where the dialog would go out of the screen at certain screen sizes or have multiple scrollbars. * Uses flexbox to ensure that the dialog content elements are always at the appropriate size. Fixes angular#2481. Fixes angular#3239. Fixes angular#6584. Fixes angular#8493.

MaximoB · 2019-05-06T15:59:37Z

This PR would fix major problems I'm having using MatDialog, any updates?

akvaliya · 2019-10-31T09:16:32Z

What is the status of this PR? When will it be completed.

charsleysa · 2020-01-09T06:00:18Z

@jelbourn is this still blocked?

jelbourn · 2020-01-09T23:50:38Z

Yes- we haven't work on this since the last update. At this point, we'll probably see if we can address this as part of our work on integrating MDC-web into the components.

AlexElin · 2021-03-03T16:17:41Z

@jelbourn have you had a chance to find out whether integrating MDC-web into the components will solve the problems that this PR intended to solve?

jelbourn · 2021-03-03T21:47:51Z

@crisbeto is this something you've looked at for the mdc dialog?

crisbeto · 2021-03-03T22:37:52Z

I haven't looked into the MDC dialog since somebody else implemented it.

josephperrott · 2021-11-19T16:15:03Z

@crisbeto Looks like this PR has fallen out of date, can you please rebase the PR.

MikaStark · 2023-08-01T14:24:49Z

Hope this PR is not abandoned and will be merged someday

crisbeto added the pr: needs review label Jan 4, 2018

crisbeto assigned jelbourn Jan 4, 2018

crisbeto requested a review from jelbourn as a code owner January 4, 2018 19:04

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 4, 2018

crisbeto force-pushed the dialog-scrollable-proper branch from 8ee04db to 214f719 Compare January 4, 2018 19:43

jelbourn reviewed Jan 11, 2018

View reviewed changes

crisbeto force-pushed the dialog-scrollable-proper branch from 214f719 to fb4dbf0 Compare January 12, 2018 18:22

jelbourn previously approved these changes Jan 12, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs review labels Jan 12, 2018

crisbeto mentioned this pull request Jan 17, 2018

fix(dialog): dialogs that hit max-width not centered on IE #8055

Merged

crisbeto force-pushed the dialog-scrollable-proper branch from fb4dbf0 to 67ab3e9 Compare January 24, 2018 17:09

crisbeto mentioned this pull request Feb 8, 2018

mat dialog actions hiding/overflow #9840

Closed

crisbeto force-pushed the dialog-scrollable-proper branch from 67ab3e9 to 2a31c21 Compare March 2, 2018 20:58

crisbeto mentioned this pull request Mar 16, 2018

feat(dialog): Ability to present MatDialog fullscreen #10094

Open

crisbeto force-pushed the dialog-scrollable-proper branch from 2a31c21 to 2c65285 Compare March 30, 2018 14:53

crisbeto force-pushed the dialog-scrollable-proper branch from 2c65285 to 3c45bee Compare April 18, 2018 19:06

andrewseguin added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 26, 2018

crisbeto force-pushed the dialog-scrollable-proper branch from 3c45bee to 0297e10 Compare July 11, 2018 16:09

crisbeto mentioned this pull request Jul 12, 2018

Dialog directives not functioning as expected when opening a dialog template with a set height #12138

Closed

crisbeto force-pushed the dialog-scrollable-proper branch from 0297e10 to d0ebb1e Compare July 12, 2018 12:54

crisbeto force-pushed the dialog-scrollable-proper branch from cc3ba6f to dccfb41 Compare March 30, 2019 13:51

mmalerba added aaa and removed aaa labels Apr 25, 2019

andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label May 30, 2019

mmalerba removed the lgtm label Jul 31, 2020

andrewseguin added the needs rebase label Aug 14, 2020

josephperrott removed the action: merge The PR is ready for merge by the caretaker label Nov 19, 2021

andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021

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

andrewseguin removed the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Mar 28, 2022

andrewseguin unassigned jelbourn Apr 22, 2022

andrewseguin dismissed jelbourn’s stale review via dccfb41 May 2, 2022 20:33

josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022

josephperrott requested a review from a team as a code owner December 18, 2024 17:40

josephperrott requested review from amysorto and mmalerba and removed request for a team December 18, 2024 17:40

mmalerba removed their request for review February 19, 2025 00:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dialog): improved handling of scrollable content #9236

fix(dialog): improved handling of scrollable content #9236

crisbeto commented Jan 4, 2018

jelbourn Jan 11, 2018

jelbourn Jan 11, 2018

crisbeto Jan 11, 2018

jelbourn Jan 11, 2018

crisbeto Jan 11, 2018

jelbourn Jan 11, 2018

crisbeto Jan 11, 2018

jelbourn Jan 11, 2018

crisbeto commented Jan 12, 2018

jelbourn left a comment

dinamix1 commented Mar 21, 2018

pharv commented Apr 23, 2018

Devvox93 commented Jun 22, 2018

ngbot bot commented Jul 11, 2018

tammysock commented Jul 17, 2018

swftvsn commented Aug 23, 2018

jpike88 commented Mar 5, 2019

MaximoB commented May 6, 2019

akvaliya commented Oct 31, 2019

charsleysa commented Jan 9, 2020

jelbourn commented Jan 9, 2020

AlexElin commented Mar 3, 2021

jelbourn commented Mar 3, 2021

crisbeto commented Mar 3, 2021

josephperrott commented Nov 19, 2021

MikaStark commented Aug 1, 2023

		@@ -1 +1,3 @@
		<ng-template cdkPortalOutlet></ng-template>
		<div class="mat-dialog-component-host">

fix(dialog): improved handling of scrollable content #9236

Are you sure you want to change the base?

fix(dialog): improved handling of scrollable content #9236

Conversation

crisbeto commented Jan 4, 2018

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 Jan 12, 2018

jelbourn left a comment

Choose a reason for hiding this comment

dinamix1 commented Mar 21, 2018

pharv commented Apr 23, 2018

Devvox93 commented Jun 22, 2018

ngbot bot commented Jul 11, 2018

tammysock commented Jul 17, 2018

swftvsn commented Aug 23, 2018

jpike88 commented Mar 5, 2019

MaximoB commented May 6, 2019

akvaliya commented Oct 31, 2019

charsleysa commented Jan 9, 2020

jelbourn commented Jan 9, 2020

AlexElin commented Mar 3, 2021

jelbourn commented Mar 3, 2021

crisbeto commented Mar 3, 2021

josephperrott commented Nov 19, 2021

MikaStark commented Aug 1, 2023