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(NgStyle): add new NgStyle directive #2665

Closed
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion gulpfile.js
Expand Up @@ -439,7 +439,7 @@ gulp.task('!test.unit.js/karma-run', function(done) {
gulp.task('test.unit.dart', function (done) {
runSequence(
'build/tree.dart',
'!build/pubget.angular2.dart',
//'!build/pubget.angular2.dart',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be commented out?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, good catch!

'!build/change_detect.dart',
'!test.unit.dart/karma-server',
'!test.unit.dart/karma-run',
Expand Down
36 changes: 36 additions & 0 deletions modules/angular2/src/directives/ng_style.ts
@@ -0,0 +1,36 @@
import {Directive, onCheck} from 'angular2/annotations';
import {ElementRef} from 'angular2/core';
import {PipeRegistry} from 'angular2/src/change_detection/pipes/pipe_registry';
import {isPresent, print} from 'angular2/src/facade/lang';
import {Renderer} from 'angular2/src/render/api';

@Directive({selector: '[ng-style]', lifecycle: [onCheck], properties: ['rawStyle: ng-style']})
export class NgStyle {
_pipe;
Copy link
Contributor

Choose a reason for hiding this comment

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

types

_rawStyle;

constructor(private _pipeRegistry: PipeRegistry, private _ngEl: ElementRef,
private _renderer: Renderer) {}

set rawStyle(v) {
this._rawStyle = v;
this._pipe = this._pipeRegistry.get('keyValDiff', this._rawStyle);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with the pipe concept yet, but does keyValDiff also support Arrays or only Maps?

(This was the Angular 1.0 behavior)

Copy link
Member Author

Choose a reason for hiding this comment

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

@matanlurey nope, keyValDiff works only with the Map-like objects. But reading your comment I'm getting an impression that you were thinking about ngClass, since I can see only maps being supported in ng1 (both JS and Dart)

If you are interested in the ngClass equivalent, the relevant PR is this: #2664)

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, ngStyle in ng1 does support expressions (not only maps).

Copy link
Member

Choose a reason for hiding this comment

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

(But only expressions that evaluate to maps, so it's not far off 😃)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this is exactly what this version does as well :-)

}

onCheck() {
var diff = this._pipe.transform(this._rawStyle);
if (isPresent(diff) && isPresent(diff.wrapped)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any benchmarks on how this could effect large Angular applications?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we've got any specific benchmarks for the directives discussed here, but my understanding is that the onCheck method should be called only when a _referenc_e to an object being watched changes - which should be pretty rare in case of ngStyle. The other part of the "speed" equation here would be performance of change detection for object properties - this I believe is already covered (?) in benchamrks. @vsavkin / @tbosch might know more.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have microbenchmarks testing transform or onCheck. But since this is how ngFor is implemented, pretty much every macrobenchmark tests it indirectly.

I don't expect the performance to be a problem here. The ".onCheck" call is monomorphic. The transform call will most likely be monomorphic too.

but my understanding is that the onCheck method should be called only when a _referenc_e to an object being watched changes

onCheck is called every single time an object is checked. onCheck is used for a deep check when all the references stay the same.

this._applyChanges(diff.wrapped);
}
}

private _applyChanges(diff): void {
diff.forEachAddedItem((record) => { this._setStyle(record.key, record.currentValue); });
diff.forEachChangedItem((record) => { this._setStyle(record.key, record.currentValue); });
diff.forEachRemovedItem((record) => { this._setStyle(record.key, null); });
}

private _setStyle(name: string, val: string): void {
this._renderer.setElementStyle(this._ngEl, name, val);
}
}
126 changes: 126 additions & 0 deletions modules/angular2/test/directives/ng_style_spec.ts
@@ -0,0 +1,126 @@
import {
AsyncTestCompleter,
beforeEach,
beforeEachBindings,
ddescribe,
xdescribe,
describe,
el,
expect,
iit,
inject,
it,
xit,
} from 'angular2/test_lib';

import {StringMapWrapper} from 'angular2/src/facade/collection';

import {Component, View} from 'angular2/angular2';

import {TestBed} from 'angular2/src/test_lib/test_bed';
import {DOM} from 'angular2/src/dom/dom_adapter';
import {NgStyle} from 'angular2/src/directives/ng_style';

export function main() {
describe('binding to CSS styles', () => {

it('should add styles specified in an object literal',
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
var template = `<div [ng-style]="{'text-align': 'right'}"></div>`;

tb.createView(TestComponent, {html: template})
.then((view) => {
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('right');

async.done();
});
}));

it('should add and change styles specified in an object expression',
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
var template = `<div [ng-style]="expr"></div>`;

tb.createView(TestComponent, {html: template})
.then((view) => {
var expr: Map<string, any>;

view.context.expr = {'text-align': 'right'};
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('right');

expr = view.context.expr;
expr['text-align'] = 'left';
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('left');

async.done();
});
}));

it('should remove styles when deleting a key in an object expression',
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
var template = `<div [ng-style]="expr"></div>`;

tb.createView(TestComponent, {html: template})
.then((view) => {
view.context.expr = {'text-align': 'right'};
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('right');

StringMapWrapper.delete(view.context.expr, 'text-align');
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('');

async.done();
});
}));

it('should co-operate with the style attribute',
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
var template = `<div style="font-size: 12px" [ng-style]="expr"></div>`;

tb.createView(TestComponent, {html: template})
.then((view) => {
view.context.expr = {'text-align': 'right'};
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('right');
expect(DOM.getStyle(view.rootNodes[0], 'font-size')).toEqual('12px');

StringMapWrapper.delete(view.context.expr, 'text-align');
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('');
expect(DOM.getStyle(view.rootNodes[0], 'font-size')).toEqual('12px');

async.done();
});
}));

it('should co-operate with the style.[styleName]="expr" special-case in the compiler',
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
var template = `<div [style.font-size.px]="12" [ng-style]="expr"></div>`;

tb.createView(TestComponent, {html: template})
.then((view) => {
view.context.expr = {'text-align': 'right'};
view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('right');
expect(DOM.getStyle(view.rootNodes[0], 'font-size')).toEqual('12px');

StringMapWrapper.delete(view.context.expr, 'text-align');
expect(DOM.getStyle(view.rootNodes[0], 'font-size')).toEqual('12px');

view.detectChanges();
expect(DOM.getStyle(view.rootNodes[0], 'text-align')).toEqual('');

async.done();
});
}));
})
}

@Component({selector: 'test-cmp'})
@View({directives: [NgStyle]})
class TestComponent {
expr;
}