feat(dialog): support minWidth, minHeight, maxWidth and maxHeight #7488

emilio-martinez · 2017-10-03T07:57:13Z

Adds Dialog support for minWidth, minHeight, maxWidth and maxHeight via config. Mostly delegates to Overlay.
Moves declared max-width on .mat-dialog-container stylesheet to be authored via MatDialogConfig, providing the same default 80vw value. Without this change, there would likely be undesired layout results due to different constraints being set on the overlay container vs the nested the dialog container.
Added note on minHeight and maxHeight regarding the potential need to also set a height due to the rules around computed height resolution (https://www.w3.org/TR/CSS2/visudet.html#min-max-widths). Any value set on height as a default would probably be assuming too much and may have varying results across browsers.

* Adds Dialog support for `minWidth`, `minHeight`, `maxWidth` and `maxHeight` via config. Mostly delegates to Overlay. * Moves declared `max-width` on `. mat-dialog-container` stylesheet to be authored via `MatDialogConfig`, providing the same default `80vw` value. Without this change, there would likely be undesired layout results due to different constraints being set on the overlay container vs the nested the dialog container. * Added note on `minHeight` and `maxHeight` regarding the potential need to also set a `height` due to the rules around computed height resolution (https://www.w3.org/TR/CSS2/visudet.html#min-max-widths). Any value set on `height` as a default would probably be assuming too much and may have varying results across browsers.

Not adding `minWidth`, `minHeight`, `maxWidth` and `maxHeight` to `config` to avoid overriding the default value of `maxWidth` particularly—the type checker will require it to be a string. Instead, setting `config` to type `MatDialogConfig` is prefered to appease the type checker since those four properties are optional.

Appeases the AOT type checker for the template by extending `MatDialogConfig` to disallow `position` from being undefined, given that it’s optional. Guards cannot be set on the template because the values are bound to NgModel.

emilio-martinez · 2017-10-03T11:13:53Z

Screenshot tests seem to be failing for components that—as far as I can tell—are unrelated to what this PR addresses. Screenshot test flagged: mdcard, mdtoolbar, and stepper.

crisbeto · 2017-10-03T18:03:22Z

src/demo-app/dialog/dialog-demo.ts

+} from '@angular/material';
+
+/**
+ * Appeases the AOT type checker for the template by extending `MatDialogConfig`


I would rather not add a separate type just for the sake of AoT. You can remove the type from the config. It'll still be type-checked when it's passed in to dialog.open.

Correct, dialog.open would type-check, but not having type results in AOT errors because the template would only have the inferred type to go off from:
Error at /home/travis/build/angular/material2/dist/packages/demo-app/dialog/dialog-demo.ngfactory.ts:526:51: Property 'minWidth' does not exist on type '{ disableClose: boolean; panelClass: string; hasBackdrop: boolean; backdropClass: string; width: ...'.
See https://travis-ci.org/angular/material2/jobs/282614008.

You can get it to play nice if you initialize the minWidth to something.

@crisbeto another alternative would be to just type with MatDialogConfig without extending, but given strictNullChecks, AOT will complain in the template about config.property possibly being undefined, so something like the below could be done. Let me know your thoughts.

<ng-template [ngIf]="config.position"> <p> <mat-form-field> <input matInput [(ngModel)]="config.position.top" (change)="config.position.bottom = ''" placeholder="Top"> </mat-form-field> <mat-form-field> <input matInput [(ngModel)]="config.position.bottom" (change)="config.position.top = ''" placeholder="Bottom"> </mat-form-field> </p> <p> <mat-form-field> <input matInput [(ngModel)]="config.position.left" (change)="config.position.right = ''" placeholder="Left"> </mat-form-field> <mat-form-field> <input matInput [(ngModel)]="config.position.right" (change)="config.position.left = ''" placeholder="Right"> </mat-form-field> </p> </ng-template>

I think that's over-complicating it. Initializing the new properties to empty strings should be fine.

Agreed re: initializing the config props. The only question I'd have is around setting the value for maxWidth since I'm initializing with 80vw on DialogConfig. Initializing it to something neutral like an empty string would override the value.

To your point about initializing, it could be as simple as initializing all min/max values with an empty string and initializing maxWidth with the 80vw, or extracting the default value out of a new DialogConfig and setting that in the maxWidth init.

Initializing all of them to an empty string (only in the demo) and the maxWidth to the same as the default is fine by me.

Cool. Addressed in 2b1c733

crisbeto · 2017-10-03T18:04:32Z

src/lib/dialog/dialog-config.ts

+  /**
+   * Min-height of the dialog.
+   * For this value to be effectively applied, `height` may also need to be defined.
+   * See https://www.w3.org/TR/CSS2/visudet.html#min-max-widths


I don't think that this needs to link to the CSS spec. Something along the lines of /** min-height of the dialog */ should suffice. Same goes for the maxHeight.

crisbeto · 2017-10-03T18:05:11Z

src/lib/dialog/dialog-config.ts

+   * For this value to be effectively applied, `height` may also need to be defined.
+   * See https://www.w3.org/TR/CSS2/visudet.html#min-max-widths
+   */
+  minHeight?: string = '';


These don't need to be initialized.

Addressed. What are your thoughts on initializing maxWidth? I left that initialized for the time being.

…axHeight

emilio-martinez · 2017-10-03T19:29:37Z

@crisbeto tests passing after your feedback with the same caveats regarding the screenshot tests noted above. Let me know if you have any further comments.

crisbeto

LGTM

jelbourn · 2017-10-04T19:33:35Z

src/lib/dialog/dialog-config.ts

+  maxWidth?: string = '80vw';
+
+  /** Max-height of the dialog */
+  maxHeight?: string;


These should all be string | number, where a number implies a pixel value to match the OverlayConfig values

jelbourn · 2017-10-04T19:34:18Z

src/lib/dialog/dialog-config.ts

+  /** Min-height of the dialog */
+  minHeight?: string;
+
+  /** Max-width of the dialog. */


This comment should include Defaults to 80vw

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-07T09:02:04Z

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

emilio-martinez added 2 commits October 3, 2017 00:18

test(dialog): add tests for minWidth, minHeight, maxWidth and maxHeight

f4c23b9

emilio-martinez requested review from crisbeto and jelbourn as code owners October 3, 2017 07:57

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

emilio-martinez force-pushed the dialog-6024 branch from 931c3be to 4473ec2 Compare October 3, 2017 08:24

emilio-martinez force-pushed the dialog-6024 branch 3 times, most recently from 9aa50df to 401e686 Compare October 3, 2017 10:26

demo(dialog): address AOT issues

3582d14

Appeases the AOT type checker for the template by extending `MatDialogConfig` to disallow `position` from being undefined, given that it’s optional. Guards cannot be set on the template because the values are bound to NgModel.

emilio-martinez force-pushed the dialog-6024 branch from 401e686 to 3582d14 Compare October 3, 2017 10:45

emilio-martinez mentioned this pull request Oct 3, 2017

dialog max width is set to 80 vw #3209

Closed

crisbeto reviewed Oct 3, 2017

View reviewed changes

emilio-martinez added 3 commits October 3, 2017 11:32

chore(dialog): address feedback regarding over-verbose comments

a30dc0b

fix(dialog): address feedback to not initialize some dialog config props

8e69f7b

demo(dialog): set init values for minWidth, minHeight, maxWidth and m…

2b1c733

…axHeight

crisbeto approved these changes Oct 3, 2017

View reviewed changes

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

jelbourn reviewed Oct 4, 2017

View reviewed changes

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

chore(dialog): allow number|string for min/max widths and heights

3b40361

jelbourn approved these changes Oct 5, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Oct 5, 2017

andrewseguin merged commit 57f19cd into angular:master Oct 9, 2017

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

feat(dialog): support minWidth, minHeight, maxWidth and maxHeight #7488

feat(dialog): support minWidth, minHeight, maxWidth and maxHeight #7488

Uh oh!

Conversation

emilio-martinez commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio-martinez commented Oct 3, 2017

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!

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!

emilio-martinez commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emilio-martinez commented Oct 3, 2017 •

edited

Loading

emilio-martinez commented Oct 3, 2017 •

edited

Loading