-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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): add hasBackdrop and backdropClass options to dialog config #2822
feat(dialog): add hasBackdrop and backdropClass options to dialog config #2822
Conversation
It looks like saucelabs timed out in the last build, not sure if someone here has the ability to redo the build. |
Reset the tests for you. |
Are there any other steps I should take for this pull request? Or is the only thing left to do wait for someone from the material2 team to take a look? |
It still needs to be reviewed. Also I can see how the |
I was actually thinking hasBackdrop is probably less useful. When hasBackdrop is false you also lose the ability to close when clicking away, which undermines disableClose. However, with backdropClass you can make the backdrop transparent and then control whether you want the ability to close or not with disableClose. backdropClass only replaces the default The library is already offering the ability to set the position of a dialog, allowing people to use it for more than just a simple standard dialog, so I think allowing custom styling of the backdrop is definitely useful. |
@crisbeto Is it possible to get this reviewed or at least discussed? Beta.3 breaks the hack I was using to have some dialogs with transparent backgrounds and it looks like at least one other person that found this is also interested (looking at the reaction to my last comment). From what I have researched the Material Guidelines don't specify/require a specific background overlay color. The API already gives the ability to customize position, width, height, so obviously it's intended to be used in many ways. The Overlay API already supports the ability to apply custom classes or just use the existing transparent class. Why should we be forced to use the translucent black overlay? This is an incredibly easy change and I can fix this PR to get it up to date with the latest master, but it would be appreciated if I could get some input from the maintainers, even if it's just a "Yes" or "No." |
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.
Mostly looks good, just one comment and also needs rebase
src/lib/dialog/dialog.ts
Outdated
@@ -161,7 +161,10 @@ export class MdDialog { | |||
let strategy = this._overlay.position().global(); | |||
let position = dialogConfig.position; | |||
|
|||
state.hasBackdrop = true; | |||
state.hasBackdrop = dialogConfig.hasBackdrop; | |||
if (dialogConfig.backdropClass && dialogConfig.backdropClass !== '') { |
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.
The extra !== ''
shouldn't be necessary
Implement hasBackdrop as laid out in the MdDialog design document. Add backdropClass option to override default `cdk-overlay-dark-backdrop` class, allowing custom styling to be applied. Closes angular#2806
5038744
to
e6a9323
Compare
@jelbourn Resolved conflicts with master and made changes based on your suggestion. |
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.
LGTM
@YeomansIII Thank you for your great job. Do we know briefly when this will be released? |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Implement hasBackdrop as laid out in the MdDialog design document. Add backdropClass option to override default
cdk-overlay-dark-backdrop
class, allowing custom styling to be applied.Closes #2806
Somewhat related #1584