Skip to content

Commit

Permalink
fix(ngClass,ngStyle): support ChangeDetectionStrategy.OnPush
Browse files Browse the repository at this point in the history 
* ngClass now properly differentiates between `ngClass` and `class`
  *  `class.<xxx>` are destructive overrides (as seen in `NgClass::klass`)
  *  `ngClass.<xxx>` use iterables to merge/enable/disable 0...n classes

@see https://plnkr.co/edit/jCICQ1GXFnzFqFTFlXwh?p=preview

fixes #206.
  • Loading branch information
@ThomasBurleson
ThomasBurleson committed Mar 27, 2017
1 parent 980d412 commit 72afd0c
Show file tree
Hide file tree
Showing 7 changed files with 318 additions and 128 deletions.
20 changes: 10 additions & 10 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,24 +36,24 @@
"node": ">= 5.4.1 < 7"
},
"dependencies": {
"@angular/common": "^2.3.5",
"@angular/compiler": "^2.3.5",
"@angular/core": "^2.3.5",
"@angular/platform-browser": "^2.3.5",
"@angular/platform-browser-dynamic": "^2.3.5",
"@angular/common": "^4.0.0",
"@angular/compiler": "^4.0.0",
"@angular/core": "^4.0.0",
"@angular/platform-browser": "^4.0.0",
"@angular/platform-browser-dynamic": "^4.0.0",
"@types/gulp-util": "^3.0.30",
"core-js": "^2.4.1",
"reflect-metadata": "^0.1.8",
"rxjs": "^5.0.1",
"systemjs": "^0.19.41",
"zone.js": "^0.7.2"
"zone.js": "^0.8.5"
},
"devDependencies": {
"@angular/compiler-cli": "^2.3.5",
"@angular/forms": "^2.3.5",
"@angular/http": "^2.3.5",
"@angular/compiler-cli": "^4.0.0",
"@angular/forms": "^4.0.0",
"@angular/http": "^4.0.0",
"@angular/material": "^2.0.0-beta.2",
"@angular/platform-server": "^2.3.5",
"@angular/platform-server": "^4.0.0",
"@angular/router": "~3.1.1",
"@angular/tsc-wrapped": "~0.4.0",
"@angularclass/conventions-loader": "^1.0.2",
Expand Down
2 changes: 1 addition & 1 deletion src/lib/flexbox/api/base-adapter.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export class MockElementRef extends ElementRef {
describe('BaseFxDirectiveAdapter class', () => {
let component;
beforeEach(() => {
component = new BaseFxDirectiveAdapter(null, new MockElementRef(), null);
component = new BaseFxDirectiveAdapter(null, null, new MockElementRef(), null);
});
describe('cacheInput', () => {
it('should call _cacheInputArray when source is an array', () => {
Expand Down
38 changes: 37 additions & 1 deletion src/lib/flexbox/api/base-adapter.ts
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,37 @@
/**
* @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
*/
import {ElementRef, Renderer} from '@angular/core';

import {BaseFxDirective} from './base';
import {ResponsiveActivation} from './../responsive/responsive-activation';
import {MediaQuerySubscriber} from '../../media-query/media-change';
import {MediaMonitor} from '../../media-query/media-monitor';


/**
* Adapter to the BaseFxDirective abstract class so it can be used via composition.
* @see BaseFxDirective
*/
export class BaseFxDirectiveAdapter extends BaseFxDirective {

/**
* Accessor to determine which @Input property is "active"
* e.g. which property value will be used.
*/
get activeKey() {
let mqa = this._mqActivation;
let key = mqa ? mqa.activatedInputKey : this._baseKey;
// ClassDirective::SimpleChanges uses 'klazz'
// instead of 'class' as a key
return (key === 'class') ? 'klazz' : key;
}

/** Hash map of all @Input keys/values defined/used */
get inputMap() {
return this._inputMap;
}
Expand All @@ -18,11 +43,22 @@ export class BaseFxDirectiveAdapter extends BaseFxDirective {
return this._mqActivation;
}

/**
* BaseFxDirectiveAdapter constructor
*/
constructor(protected _baseKey: string, // non-responsive @Input property name
protected _mediaMonitor: MediaMonitor,
protected _elementRef: ElementRef,
protected _renderer: Renderer ) {
super(_mediaMonitor, _elementRef, _renderer);
}


/**
* @see BaseFxDirective._queryInput
*/
queryInput(key) {
return this._queryInput(key);
return key ? this._queryInput(key) : undefined;
}

/**
Expand Down
44 changes: 25 additions & 19 deletions src/lib/flexbox/api/base.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,10 @@ export type StyleDefinition = string|{[property: string]: string|number};

/** Abstract base class for the Layout API styling directives. */
export abstract class BaseFxDirective implements OnDestroy {
/**
* Original dom Elements CSS display style
*/
protected _display;

/**
* MediaQuery Activation Tracker
*/
protected _mqActivation: ResponsiveActivation;

/**
* Dictionary of input keys with associated values
*/
protected _inputMap = {};
get hasMediaQueryListener() {
return !!this._mqActivation;
}

/**
*
Expand All @@ -46,6 +36,7 @@ export abstract class BaseFxDirective implements OnDestroy {
this._display = this._getDisplayStyle();
}


// *********************************************
// Accessor Methods
// *********************************************
Expand Down Expand Up @@ -172,12 +163,15 @@ export abstract class BaseFxDirective implements OnDestroy {
protected _listenForMediaQueryChanges(key: string,
defaultValue: any,
onMediaQueryChange: MediaQuerySubscriber): ResponsiveActivation { // tslint:disable-line:max-line-length
let keyOptions = new KeyOptions(key, defaultValue, this._inputMap);
return this._mqActivation = new ResponsiveActivation(
keyOptions,
this._mediaMonitor,
(change) => onMediaQueryChange.call(this, change)
);
if ( !this._mqActivation ) {
let keyOptions = new KeyOptions(key, defaultValue, this._inputMap);
this._mqActivation = new ResponsiveActivation(
keyOptions,
this._mediaMonitor,
(change) => onMediaQueryChange(change)
);
}
return this._mqActivation;
}

/**
Expand All @@ -201,4 +195,16 @@ export abstract class BaseFxDirective implements OnDestroy {
return this._mqActivation.hasKeyValue(key);
}

/** Original dom Elements CSS display style */
protected _display;

/**
* MediaQuery Activation Tracker
*/
protected _mqActivation: ResponsiveActivation;

/**
* Dictionary of input keys with associated values
*/
protected _inputMap = {};
}
94 changes: 81 additions & 13 deletions src/lib/flexbox/api/class.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,73 @@ describe('class directive', () => {
});
});

it('should keep existing class selector', () => {
it('should merge `ngClass` values with any `class` values', () => {
fixture = createTestComponent(`
<div class="existing-class" ngClass.xs="xs-class">
<div class="class0" ngClass="class1 class2">
</div>
`);

expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).toHaveCssClass('class1');
expectNativeEl(fixture).toHaveCssClass('class2');
});

it('should override base `class` values with responsive values', () => {
fixture = createTestComponent(`
<div class="class0"
class.xs="class1 class2"
ngClass.xs="what">
</div>
`);

expectNativeEl(fixture).toHaveCssClass('class0');
expectNativeEl(fixture).not.toHaveCssClass('class1');
expectNativeEl(fixture).not.toHaveCssClass('class2');

activateMediaQuery('xs');
expectNativeEl(fixture).not.toHaveCssClass('class0');
expectNativeEl(fixture).toHaveCssClass('class1');
expectNativeEl(fixture).toHaveCssClass('class2');

// activateMediaQuery('lg');
// expectNativeEl(fixture).toHaveCssClass('class0');
// expectNativeEl(fixture).not.toHaveCssClass('class1');
// expectNativeEl(fixture).not.toHaveCssClass('class2');
});

it('should keep the raw existing `class` with responsive updates', () => {
fixture = createTestComponent(`
<div class="existing-class" ngClass="class1" ngClass.xs="xs-class">
</div>
`);

expectNativeEl(fixture).toHaveCssClass('existing-class');
expectNativeEl(fixture).toHaveCssClass('class1');

activateMediaQuery('xs');
expectNativeEl(fixture).toHaveCssClass('xs-class');
expectNativeEl(fixture).toHaveCssClass('existing-class');
expectNativeEl(fixture).not.toHaveCssClass('class1');

activateMediaQuery('lg');
expectNativeEl(fixture).not.toHaveCssClass('xs-class');
expectNativeEl(fixture).toHaveCssClass('existing-class');
expectNativeEl(fixture).toHaveCssClass('class1');
});


it('should keep allow removal of class selector', () => {
fixture = createTestComponent(`
<div
class="existing-class"
[ngClass.xs]="{'xs-class':true, 'existing-class':false}">
</div>
`);

expectNativeEl(fixture).toHaveCssClass('existing-class');
activateMediaQuery('xs');
expectNativeEl(fixture).not.toHaveCssClass('existing-class');
expectNativeEl(fixture).toHaveCssClass('xs-class');

activateMediaQuery('lg');
expectNativeEl(fixture).toHaveCssClass('existing-class');
Expand All @@ -83,15 +141,22 @@ describe('class directive', () => {
it('should keep existing ngClass selector', () => {
// @see documentation for @angular/core ngClass =http://bit.ly/2mz0LAa
fixture = createTestComponent(`
<div ngClass="existing-class" ngClass.xs="existing-class xs-class">
<div class="always"
ngClass="existing-class"
ngClass.xs="existing-class xs-class">
</div>
`);

expectNativeEl(fixture).toHaveCssClass('always');
expectNativeEl(fixture).toHaveCssClass('existing-class');

activateMediaQuery('xs');
expectNativeEl(fixture).toHaveCssClass('always');
expectNativeEl(fixture).toHaveCssClass('existing-class');
expectNativeEl(fixture).toHaveCssClass('xs-class');

activateMediaQuery('lg');
expectNativeEl(fixture).toHaveCssClass('always');
expectNativeEl(fixture).toHaveCssClass('existing-class');
expectNativeEl(fixture).not.toHaveCssClass('xs-class');
});
Expand All @@ -112,21 +177,23 @@ describe('class directive', () => {

it('should work with ngClass object notation', () => {
fixture = createTestComponent(`
<div [ngClass]="{'xs-1': hasXs1, 'xs-3': hasXs3}"
[ngClass.xs]="{'xs-1': hasXs1, 'xs-2': hasXs2}">
<div [ngClass]="{'x1': hasX1, 'x3': hasX3}"
[ngClass.xs]="{'x1': hasX1, 'x2': hasX2}">
</div>
`);
activateMediaQuery('xs');
expectNativeEl(fixture, {hasXs1: true, hasXs2: false}).toHaveCssClass('xs-1');
expectNativeEl(fixture, {hasXs1: true, hasXs2: false}).not.toHaveCssClass('xs-2');
expectNativeEl(fixture, {hasX1: true, hasX2: true, hasX3: true}).toHaveCssClass('x1');
expectNativeEl(fixture, {hasX1: true, hasX2: true, hasX3: true}).not.toHaveCssClass('x2');
expectNativeEl(fixture, {hasX1: true, hasX2: true, hasX3: true}).toHaveCssClass('x3');

expectNativeEl(fixture, {hasXs1: false, hasXs2: true}).toHaveCssClass('xs-2');
expectNativeEl(fixture, {hasXs1: false, hasXs2: true}).not.toHaveCssClass('xs-1');
activateMediaQuery('X');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: false}).toHaveCssClass('x1');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: false}).not.toHaveCssClass('x2');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: false}).not.toHaveCssClass('x3');

activateMediaQuery('md');
expectNativeEl(fixture, {hasXs1: true, hasX2: false, hasXs3: true}).toHaveCssClass('xs-3');
expectNativeEl(fixture, {hasXs1: true, hasX2: false, hasXs3: true}).not.toHaveCssClass('xs-2');
expectNativeEl(fixture, {hasXs1: true, hasX2: false, hasXs3: true}).toHaveCssClass('xs-1');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: true}).toHaveCssClass('x1');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: true}).not.toHaveCssClass('x2');
expectNativeEl(fixture, {hasX1: true, hasX2: false, hasX3: true}).toHaveCssClass('x3');
});

it('should work with ngClass array notation', () => {
Expand All @@ -151,6 +218,7 @@ describe('class directive', () => {
export class TestClassComponent implements OnInit {
hasXs1: boolean;
hasXs2: boolean;
hasXs3: boolean;

constructor(private media: ObservableMedia) {
}
Expand Down

0 comments on commit 72afd0c

Please sign in to comment.