Skip to content

chore(api): overlay class audits #6372 #7454

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions src/cdk/css/css.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

/* Parses arguments into a ngClass-like object */
export function parseClassList(classList:
Copy link
Member

@crisbeto crisbeto Oct 1, 2017

Choose a reason for hiding this comment

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

I'm sure that we need to support all of these formats. NgClass has them, presumably, because the expressions are dynamic and there a lot of ways to combine static classes and dynamic ones. In our case we only add the classes on init and we don't care about updating them, which can be done just as easily with an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original ticket expressed desire to standardize this format. Ref: #6372

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW NgClass seemed like a natural thing to support for template-based overlays like select, menu, autocomplete, tooltip. For dialog and snackbar, it's probably less important, but there are some good use cases for having the ability to dynamically update classes, #6206 (comment).

Further, I would personally find an NgClass cdk utility useful for custom overlay-based components!

Copy link
Member

Choose a reason for hiding this comment

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

Template-based overlays should "just work" as they are at the moment (you can stick an NgClass on an element inside the TemplateRef). This PR adds a similar signature to the programmatic API, but it's not really NgClass because it won't update dynamically.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok tbh, I hadn't actually read the pr 😄 . My initial concern in filing #6372 was how snackbar and dialog differed in support and naming. Combined with little type info available in the docs (as #6947 is trying to solve) it's hard to anticipate exactly what will happen just by looking at the docs.

I agree, {[klass: string]: any} is probably extraneous (and maybe misrepresenting) if classes don't update dynamically. While I still think that's a valid feature via #6206, maybe for another time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, so basically this PR:

  • Unifies all the css class names to panelClass
  • Normalizes all the signatures to ngClass-like

That means you can pass it an string, string array or object key/value pair. In the cases of the snackbar and overlay, there is no ngClass used so it just does a evaluate of the condition and matches it. I personally think the consistent signature is valuable and in the situation where there isn't real ngClass I could do an actual implementation I just didn't do it because I didn't know how far we wanted to take this.

@jelbourn - Can you advise on next steps with this? Currently the PR is just in a evaluation phase and I know you expressed concerns with adding another CDK lib here too so I didn't want to take it too far without first review.

undefined|string|string[]|Set<string>|{[klass: string]: any}) {
let klasses = {};

if (classList) {
if (Array.isArray(classList) || classList instanceof Set) {
(<any>classList).forEach((klass: string) => klasses[klass] = true);
} else if (typeof classList === 'string') {
klasses = (<string>classList).split(' ').reduce((obj: any, className: string) => {
obj[className] = true;
return obj;
}, {});
}
}

return klasses;
}

/** Parses a ngClass-like object into an array of strings */
export function getClassList(classList: {[klass: string]: any}): string[] {
return Object.keys(classList).reduce((arr: string[], className: string) => {
if (classList[className]) {
arr.push(className);
}
return arr;
}, []);
}

/** Parses a ngClass-like object into an array of strings */
export function parseAndGetClassList(
klasses: undefined|string|string[]|Set<string>|{[klass: string]: any}) {
return getClassList(parseClassList(klasses));
}
1 change: 1 addition & 0 deletions src/cdk/css/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './css';
9 changes: 9 additions & 0 deletions src/cdk/css/public_api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

export * from './css';
13 changes: 13 additions & 0 deletions src/cdk/css/tsconfig-build.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"extends": "../tsconfig-build",
"files": [
"public_api.ts"
],
"angularCompilerOptions": {
"annotateForClosureCompiler": true,
"strictMetadataEmit": true,
"flatModuleOutFile": "index.js",
"flatModuleId": "@angular/cdk/css",
"skipTemplateCodegen": true
}
}
2 changes: 1 addition & 1 deletion src/cdk/overlay/overlay-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export class OverlayConfig {
scrollStrategy?: ScrollStrategy = new NoopScrollStrategy();

/** Custom class to add to the overlay pane. */
panelClass?: string | string[] = '';
panelClass?: string|string[]|Set<string>|{[klass: string]: any};

/** Whether the overlay has a backdrop. */
hasBackdrop?: boolean = false;
Expand Down
7 changes: 2 additions & 5 deletions src/cdk/overlay/overlay-ref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {PortalHost, Portal} from '@angular/cdk/portal';
import {OverlayConfig} from './overlay-config';
import {Observable} from 'rxjs/Observable';
import {Subject} from 'rxjs/Subject';
import {parseAndGetClassList} from '@angular/cdk/css';
import {first} from 'rxjs/operator/first';


Expand Down Expand Up @@ -77,11 +78,7 @@ export class OverlayRef implements PortalHost {

if (this._config.panelClass) {
// We can't do a spread here, because IE doesn't support setting multiple classes.
if (Array.isArray(this._config.panelClass)) {
this._config.panelClass.forEach(cls => this._pane.classList.add(cls));
} else {
this._pane.classList.add(this._config.panelClass);
}
parseAndGetClassList(this._config.panelClass).forEach(cls => this._pane.classList.add(cls));
}

// Only emit the `attachments` event once all other setup is done.
Expand Down
1 change: 1 addition & 0 deletions src/lib/datepicker/datepicker-content.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<mat-calendar cdkTrapFocus
[ngClass]="datepicker.panelClass"
[id]="datepicker.id"
[startAt]="datepicker.startAt"
[startView]="datepicker.startView"
Expand Down
3 changes: 3 additions & 0 deletions src/lib/datepicker/datepicker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ export class MatDatepicker<D> implements OnDestroy {
*/
@Input() touchUi = false;

/** Classes to be passed to the date picker panel. Supports the same syntax as `ngClass`. */
@Input() panelClass: string|string[]|Set<string>|{[key: string]: any};

/** Whether the datepicker pop-up should be disabled. */
@Input()
get disabled() {
Expand Down
2 changes: 1 addition & 1 deletion src/lib/dialog/dialog-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class MatDialogConfig<D = any> {
role?: DialogRole = 'dialog';

/** Custom class for the overlay pane. */
panelClass?: string | string[] = '';
panelClass?: string|string[]|Set<string>|{[klass: string]: any} = '';

/** Whether the dialog has a backdrop. */
hasBackdrop?: boolean = true;
Expand Down
20 changes: 13 additions & 7 deletions src/lib/menu/menu-directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {throwMatMenuInvalidPositionX, throwMatMenuInvalidPositionY} from './menu
import {MatMenuItem} from './menu-item';
import {MatMenuPanel} from './menu-panel';
import {MenuPositionX, MenuPositionY} from './menu-positions';
import {parseClassList} from '@angular/cdk/css';


/** Default `mat-menu` options that can be overridden. */
Expand Down Expand Up @@ -127,15 +128,20 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {
* menu template that displays in the overlay container. Otherwise, it's difficult
* to style the containing menu from outside the component.
* @param classes list of class names
* @deprecated Use `panelClass` instead.
*/
@Input('class')
set classList(classes: string) {
if (classes && classes.length) {
this._classList = classes.split(' ').reduce((obj: any, className: string) => {
obj[className] = true;
return obj;
}, {});
@Input('class') classList = this.panelClass;

/**
* This method takes classes set on the host mat-menu element and applies them on the
* menu template that displays in the overlay container. Otherwise, it's difficult
* to style the containing menu from outside the component.
* @param classes list of class names
*/
@Input()
set panelClass(classList: string|string[]|Set<string>|{[klass: string]: any}) {
if (classList) {
this._classList = parseClassList(classList);
this._elementRef.nativeElement.className = '';
this.setPositionClasses();
}
Expand Down
8 changes: 7 additions & 1 deletion src/lib/snack-bar/snack-bar-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,15 @@ export class MatSnackBarConfig {
/** The length of time in milliseconds to wait before automatically dismissing the snack bar. */
duration?: number = 0;

/** Extra CSS classes to be added to the snack bar container. */
/**
* Extra CSS classes to be added to the snack bar container.
* @deprecated Use `panelClass` instead.
*/
extraClasses?: string[];

/** Extra CSS classes to be added to the snack bar container. */
panelClass?: string|string[]|Set<string>|{[klass: string]: any};

/** Text layout direction for the snack bar. */
direction?: Direction = 'ltr';

Expand Down
11 changes: 5 additions & 6 deletions src/lib/snack-bar/snack-bar-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {AnimationCurves, AnimationDurations} from '@angular/material/core';
import {Observable} from 'rxjs/Observable';
import {Subject} from 'rxjs/Subject';
import {MatSnackBarConfig} from './snack-bar-config';
import {parseAndGetClassList} from '@angular/cdk/css';


export const SHOW_ANIMATION =
Expand Down Expand Up @@ -104,12 +105,10 @@ export class MatSnackBarContainer extends BasePortalHost implements OnDestroy {
throw Error('Attempting to attach snack bar content after content is already attached');
}

if (this.snackBarConfig.extraClasses) {
// Not the most efficient way of adding classes, but the renderer doesn't allow us
// to pass in an array or a space-separated list.
for (let cssClass of this.snackBarConfig.extraClasses) {
this._renderer.addClass(this._elementRef.nativeElement, cssClass);
}
if (this.snackBarConfig.extraClasses || this.snackBarConfig.panelClass) {
const klasses = this.snackBarConfig.extraClasses || this.snackBarConfig.panelClass;
parseAndGetClassList(klasses)
.forEach(klass => this._renderer.addClass(this._elementRef.nativeElement, klass));
}

if (this.snackBarConfig.horizontalPosition === 'center') {
Expand Down