-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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.
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
feat(dialog): support minWidth, minHeight, maxWidth and maxHeight #7488
Changes from 4 commits
43d66d6
f4c23b9
0cc61ae
3582d14
a30dc0b
8e69f7b
2b1c733
3b40361
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,26 @@ export class MatDialogConfig { | |
/** Height of the dialog. */ | ||
height?: string = ''; | ||
|
||
/** Min-width of the dialog. */ | ||
minWidth?: string = ''; | ||
|
||
/** | ||
* 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that this needs to link to the CSS spec. Something along the lines of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
*/ | ||
minHeight?: string = ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These don't need to be initialized. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed. What are your thoughts on initializing |
||
|
||
/** Max-width of the dialog. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment should include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
maxWidth?: string = '80vw'; | ||
|
||
/** | ||
* Max-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 | ||
*/ | ||
maxHeight?: string = ''; | ||
|
||
/** Position overrides. */ | ||
position?: DialogPosition; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 todialog.open
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can get it to play nice if you initialize the
minWidth
to something.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto another alternative would be to just type with
MatDialogConfig
without extending, but given strictNullChecks, AOT will complain in the template aboutconfig.property
possibly beingundefined
, so something like the below could be done. Let me know your thoughts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's over-complicating it. Initializing the new properties to empty strings should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed re: initializing the
config
props. The only question I'd have is around setting the value formaxWidth
since I'm initializing with80vw
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 the80vw
, or extracting the default value out of a newDialogConfig
and setting that in themaxWidth
init.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Addressed in 2b1c733