Skip to content
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): custom class option #4718 #4012 #4722

Merged
merged 4 commits into from
May 30, 2017

Conversation

jbojcic1
Copy link
Contributor

Extend dialog config options to allow custom dialog class. Custom class enables media queries.

Extend dialog config options to allow custom dialog class. Custom class enables media queries.

angular#4718 angular#4012
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 22, 2017
@jbojcic1
Copy link
Contributor Author

jbojcic1 commented May 22, 2017

I tried to run unit tests but I am getting chrome disconnected error message so it seems something is wrong with karma config. I've tried to increase timeouts but it didn't help. Also e2e test were failing even before my changes so couldn't test that either.

@willshowell
Copy link
Contributor

Would it be possible to support the same syntax as ngClass?

e.g. #4629

@jbojcic1
Copy link
Contributor Author

jbojcic1 commented May 23, 2017

@willshowell that would be great though situation here is more complex as pane element is created dynamically so I'd need to handle all different types of class object to add classes with classList.add. I'll try

@willshowell
Copy link
Contributor

Ah yes I see.

I'm curious why you chose to add the class to the cdk-overlay-pane. Since the overlay pane has so few styles, it would seem that attaching the custom class to mat-dialog-container would be more useful (though happy to be shown otherwise).

If you instead chose to attach to mat-dialog-container, then you could do the following. It wouldn't give you ngClass compatibility (though @jelbourn has an open issue angular/angular#7289 for that), but would remove your need to manually set/remove classes from the overlay pane.

// dialog-container.ts

@Component({
  // ...
  host: {
+   '[class]': 'dialogConfig?.dialogClass',
    '[class.mat-dialog-container]': 'true',
    '[attr.role]': 'dialogConfig?.role',
    '[@slideDialog]': '_state',
    '(@slideDialog.done)': '_onAnimationDone($event)',
  },
})
export class MdDialogContainer extends BasePortalHost {

@@ -15,6 +15,9 @@ export class OverlayState {
/** Strategy to be used when handling scroll events while the overlay is open. */
scrollStrategy: ScrollStrategy = new NoopScrollStrategy();

/** Custom class to add to the dialog */
dialogClass: string = '';
Copy link
Member

Choose a reason for hiding this comment

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

Overlay is the basis for multiple components, not just dialog. I would call this panelClass.

@@ -27,6 +27,9 @@ export class MdDialogConfig {
/** The ARIA role of the dialog element. */
role?: DialogRole = 'dialog';

/** Custom class for the dialog, */
dialogClass?: string = '';
Copy link
Member

Choose a reason for hiding this comment

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

Probably call this panelClass as well (this is already DialogConfig, so using the word dialog again on a property is redundant)

@@ -53,6 +53,10 @@ export class OverlayRef implements PortalHost {
this._attachBackdrop();
}

if (this._state.dialogClass) {
Copy link
Member

Choose a reason for hiding this comment

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

There needs to be a unit test for this behavior in the overlay tests

@@ -69,6 +73,9 @@ export class OverlayRef implements PortalHost {
this._togglePointerEvents(false);
this._state.scrollStrategy.disable();
this._detachments.next();
if (this._state.dialogClass) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think removing the class is necessary

@jbojcic1
Copy link
Contributor Author

@willshowell I chose cdk-overlay-pane because when you use width dialog option it's set to that element so I guess that's the right one.

@willshowell
Copy link
Contributor

@jbojcic1 understood, and I agree that it keeps it consistent. My personal use cases were for removing the dialog padding and overriding the dialog's max-width (see #4653). But after playing with it some, I see that you get better position control by having the selector on the cdk-overlay-pane 👍

Thanks for putting this PR together and once it's merged, I'll just use .custom-overlay .mat-dialog-container to accomplish those overrides.

Changed custom class config option name. Remove unnecessary class removal on detach.

angular#4718 angular#4012
Added missing unit test to check if overlay pane has custom panel class.

angular#4718 angular#4012
@jbojcic1
Copy link
Contributor Author

@jelbourn pls check now

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Just a couple last comments

@@ -15,6 +15,9 @@ export class OverlayState {
/** Strategy to be used when handling scroll events while the overlay is open. */
scrollStrategy: ScrollStrategy = new NoopScrollStrategy();

/** Custom class to add to the dialog */
Copy link
Member

Choose a reason for hiding this comment

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

Comment still says "dialog", should say "overlay pane"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot that one :)

beforeEach(() => {
config = new OverlayState();
config.panelClass = 'custom-panel-class';
});
Copy link
Member

Choose a reason for hiding this comment

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

You don't really need the beforeEach here; you can create the stats and set the class in the test itself because it's only being used once

Change wrong comment. Remove unnecessary beforeEach from test.

angular#4718 angular#4012
@jbojcic1
Copy link
Contributor Author

@jelbourn done

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels May 24, 2017
@fobaz
Copy link

fobaz commented May 25, 2017

FYI, somebody is very much waiting for this merge.

@jonsamwell
Copy link

Any update when this will make it into a release?

@jbojcic1
Copy link
Contributor Author

@jonsamwell not sure. Maybe @jelbourn will know?

@morkro
Copy link

morkro commented May 30, 2017

Very much looking forward to this 😬

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants