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.

Sign up for GitHub

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

[Bugfix Beta] Fix attrs in className and attribute bindings #14384

Merged
merged 1 commit into from
Sep 29, 2016
Merged

[Bugfix Beta] Fix attrs in className and attribute bindings #14384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions packages/ember-glimmer/lib/utils/bindings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ function referenceForKey(component, key) {
}

function referenceForParts(component, parts) {
let isAttrs = parts[0] === 'attrs';
Copy link
Contributor

Choose a reason for hiding this comment

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

is parts guaranteed to be length 1? is it very important that we never index outside of the array on get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream from this the check is done to make sure we have a path reference.


// TODO deprecate this
if (isAttrs) {
parts.shift();

if (parts.length === 1) {
return referenceForKey(component, parts[0]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the else here? is it supposed to fall through or assert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want this to fall through. So given attrs.foo we want to delegate to referenceForKey, however if we shift off the value and we still have a path e.g. parts.length !== 1 (guranteed to never be 0) then we want to still create a path ref.

}

return referenceFromParts(component[ROOT_REF], parts);
}

Expand Down
31 changes: 31 additions & 0 deletions packages/ember-glimmer/tests/integration/components/attribute-bindings-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,37 @@ moduleFor('Attribute bindings integration', class extends RenderingTest {
this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'data-foo': 'foo', 'data-bar': 'bar' }, content: 'hello' });
}

['@test it can have attribute bindings with attrs']() {
let FooBarComponent = Component.extend({
attributeBindings: ['attrs.foo:data-foo', 'attrs.baz.bar:data-bar']
});

this.registerComponent('foo-bar', { ComponentClass: FooBarComponent, template: 'hello' });

this.render('{{foo-bar foo=model.foo baz=model.baz}}', {
model: { foo: undefined, baz: { bar: 'bar' } }
});

this.assertComponentElement(this.firstChild, { tagName: 'div', content: 'hello', attrs: { 'data-bar': 'bar' } });

this.runTask(() => this.rerender());

this.assertComponentElement(this.firstChild, { tagName: 'div', content: 'hello', attrs: { 'data-bar': 'bar' } });

this.runTask(() => {
set(this.context, 'model.foo', 'foo');
set(this.context, 'model.baz.bar', undefined);
});

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'data-foo': 'foo' }, content: 'hello' });

this.runTask(() => set(this.context, 'model', {
foo: undefined, baz: { bar: 'bar' }
}));

this.assertComponentElement(this.firstChild, { tagName: 'div', content: 'hello', attrs: { 'data-bar': 'bar' } });
}

['@test it can have attribute bindings with a nested path']() {
let FooBarComponent = Component.extend({
attributeBindings: ['foo.bar:data-foo-bar']
Expand Down
31 changes: 31 additions & 0 deletions packages/ember-glimmer/tests/integration/components/class-bindings-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,37 @@ moduleFor('ClassNameBindings integration', class extends RenderingTest {
this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'class': classes('ember-view foo enabled sad') }, content: 'hello' });
}

['@test attrs in classNameBindings']() {
let FooBarComponent = Component.extend({
classNameBindings: ['attrs.joker:purple:green', 'attrs.batman.robin:black:red']
});

this.registerComponent('foo-bar', { ComponentClass: FooBarComponent, template: 'hello' });

this.render('{{foo-bar joker=model.wat batman=model.super}}', {
model: { wat: false, super: { robin: true } }
});

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'class': classes('ember-view green black') }, content: 'hello' });

this.runTask(() => this.rerender());

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'class': classes('ember-view green black') }, content: 'hello' });

this.runTask(() => {
set(this.context, 'model.wat', true);
set(this.context, 'model.super.robin', false);
});

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'class': classes('ember-view purple red') }, content: 'hello' });

this.runTask(() => set(this.context, 'model', {
wat: false, super: { robin: true }
}));

this.assertComponentElement(this.firstChild, { tagName: 'div', attrs: { 'class': classes('ember-view green black') }, content: 'hello' });
}

['@test it can have class name bindings in the template']() {
this.registerComponent('foo-bar', { template: 'hello' });

Expand Down