Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Evaluate class bindings for the view helper within the appropriate context #593

Merged
merged 1 commit into from

5 participants

@dgeb

Different rules are currently applied to bindings for regular attributes vs. classes within the view helper. Regular attribute bindings are evaluated in the binding context, while class bindings are evaluated in the view context.

I've illustrated this in this fiddle - see the TextField examples. The first works but is inconsistent, and the second doesn't work even though it's consistent. I included an example using bindAttr for the sake of comparison.

This PR ensures that class bindings will be evaluated using the same rules as other bindings. I've also added tests for various types of bindings in view helpers (view helper tests didn't exist before).

@wagenet
Owner

Looks like a good change.

@dgeb

I realized that I missed the case in which classes are directly evaluated (e.g. classBinding="isDone" or classBinding="customClass"). I'm working to add these to this PR, but this leads to some other tests failing for collection views which I need to investigate. I'll update this soon...

@dgeb

I've added the changes needed to support direct binding to classes, which works with either strings or booleans (e.g. classBinding="isDone" -> class="is-done").

There is still one failing collection_view_test which needs to be resolved.

@dgeb

I've just introduced a new context specifier to provide more flexibility within the {{view}} helpers: childView.

Why is this needed? Since bindings for class (i.e. classBinding) and other attributes (e.g. valueBinding) are evaluated in the binding context (or global path, if appropriate), there is no way to set a binding within the context of the embedded "child" view from a handlebars template. This might be useful for, say, a table view that wants to display row views with a particular class dependent upon those row views. The {{collection}} helper also needs this ability when creating item views that have itemClassBinding.

I chose the name childView to be consistent with plans to introduce a view context that would allow evaluation in the containing view's context, regardless of the binding context.

Last but not least, all tests now pass :)

@wagenet
Owner

@dgeb This doesn't merge cleanly. Do you mind rebasing?

@dgeb

@wagenet I just rebased - it should merge now.

BTW, I moved my new view tests into handlebars_test.js to be with similar tests rather than keeping them in a separate file. However, handlebars_test.js has gotten so big (> 1000 lines) that it might make sense to break the whole thing apart soon. I could tackle this in a separate PR if you agree.

@wagenet
Owner

@dgeb Definitely agree that we should split apart handlebars_test.js.

@wagenet
Owner

@dgeb I think you talked with @wcyats about this some. What was his opinion on it?

@dgeb

@wagenet - When I discussed this issue with @wycats, he said that his preference was to make the context always be the binding context, and to introduce a view prefix to always reference the current view.

For instance:

{{#with App.formController}}
  {{#view App.FormView}}
    {{view Ember.TextField valueBinding="title"}}
    {{view Ember.TextArea valueBinding="body"}}
  {{/view}}
{{/with}}

In this example, the bindings reference the #with block. In order to reference the view, it would be necessary to say view.title.

This makes a lot of sense to me and does not conflict with anything in this PR. However, I am introducing one new concept that @wycats did not express an opinion on: the ability to reference a property of a child view from a binding. In this PR, this is done with the childView prefix.

Here's an example where it might be useful:

<table>
  {{#each tasks}}
  {{view App.TaskView classBinding="childView.isEditable:editable" taskBinding="this"}}
  {{/each}}
</table>

Here isEditable is a property of App.TaskView, and can only be accessed from this template via childView.isEditable.

Although I don't see it getting a lot of use, I think the childView prefix is important to include and the name seems consistent with the view prefix suggested by @wycats.

@dgeb

I'd hate to see this PR derailed or delayed because of the introduction of the childView prefix. However, without the addition of this concept, there is still one failing test for {{collection}}.

{{#collection "TemplateTests.itemClassBindingTestCollectionView" itemClassBinding="content.isBaz"}}foo{{/collection}}')

In this case, content.isBaz is intended to reference a property of the item, and not the binding context.

I can think of a few options:

1) Choose a general purpose prefix other than childView, but with the same intent. For example, innerView.content.isBaz, inner.content.isBaz or simply .content.isBaz

2) Choose a prefix that applies only to items in a {{collection}}. For example, item.content.isBaz. Don't allow other bindings to be evaluated in the context of an inserted {{view}}.

3) Make a special case for items in a {{collection}}, so that bindings are always evaluated in the context of the item view, rather than the binding context.

I find (2) and (3) a bit weird / inconsistent, but still preferable to the major inconsistency fixed with this PR.

@wagenet
Owner

childView seems ok to me, but I'd definitely like some feedback from @wycats and @tomdale on this. I don't want to add any new API unless we're all generally agreed that it's a good idea.

@dgeb

@wagenet @wycats @tomdale - One more try! How about using @each within {{collection}} to reference each item view? Seems consistent with Ember.Array, right?

@tomdale
Owner

@dgeb Hey Dan, does c94ccdc help you at all? Anything referencing a view. path should resolve to the referenced view…

@dgeb

@tomdale Sadly, that change does not help with this issue, despite all the goodness it brings to context-referencing. The view. path, when specified directly on a view element, references the containing view and not the view itself. I'm talking about the need for templates to be able to reference a view's property from within the view's declaration.

Let me use your example to illustrate what I'm talking about:

{{#each App.photosController}}
  Photo Title: {{title}}
  {{#view App.InfoView}}
    {{date}}
    {{cameraType}}
    {{view.otherViewProperty}}
  {{/view}}
{{/each}}

Let's say that we want to bind a class to each App.InfoView based upon a property of the view itself. If we were to say {{#view App.InfoView classBinding="view.isHighlighted:hi"}}, then view.isHighlighted would reference the view that contains App.InfoView.

However, a prefix such as innerView., childView., inner., self. or even just . could be used to reference a property of the App.InfoView itself:

{{#each App.photosController}}
  Photo Title: {{title}}
  {{#view App.InfoView classBinding=".isHighlighted:hi"}}
    {{date}}
    {{cameraType}}
    {{view.otherViewProperty}}
  {{/view}}
{{/each}}
@tomdale
Owner

Ahh, interesting. This should definitely be possible without too much work I think.

@tomdale
Owner

What semantics does the path this.fooProperty have in this context?

@dgeb

@tomdale Currently this.fooProperty references fooProperty within the current binding context (and not the view being declared). I'm not sure we can get away with re-purposing this without providing an alternative way to bind a property in a view to the current binding context. In other words, we still need to handle photoBinding="this" here:

{{#each App.photosController}}
  Photo Title: {{title}}
  {{#view App.InfoView photoBinding="this"}}
    {{date}}
    {{cameraType}}
    {{view.otherViewProperty}}
  {{/view}}
{{/each}}

With that said, if you're up for some breaking changes, maybe all of this can be neatened up together :)

@dgeb

For example, context could be introduced as a replacement for the current way this is used, and this could be used to refer to the element being declared. This would allow:

{{#each App.photosController}}
  Photo Title: {{title}}
  {{#view App.InfoView photoBinding="context" classBinding="this.isHighlighted:hi"}}
    {{date}}
    {{cameraType}}
    {{view.otherViewProperty}}
  {{/view}}
{{/each}}
@tomdale
Owner

@dgeb I like that a lot, actually.

@wagenet
Owner

This makes sense, but it breaks compatibility with standard Handlebars. Are we ok with this?

@dgeb

To preserve handlebars compatibility as much as possible, we could allow both this and context to refer to the current binding context. However, within a block helper, like {{view}}, this would refer to the block element instead of the current binding context.

This would allow our example to include an array of tags as follows:

{{#each App.photosController}}
  Photo Title: {{title}}
  {{#view App.InfoView photoBinding="context" classBinding="this.isHighlighted:hi"}}
    {{date}}
    {{cameraType}}
    {{view.otherViewProperty}}
    <ul>{{#each tags}}<li>{{this}}</li>{{/each}}</ul>
  {{/view}}
{{/each}}
@dgeb

Upon reflection, I'm uncomfortable with breaking standard handlebars compatibility for the sake of redefining the usage of this. It seems desirable to be able to ship ember without handlebars at some point, so keeping standard handlebars compatibility is preferable.

Given the introduction of the view scope, I think this problem should be solved narrowly with the introduction of a new scope that includes view in its name. Given the discussion above, I think thisView is the best candidate. It clarifies that the scope is self-referential AND a view.

If you agree, just say the word and I'll update this p.r. (I've got some rebased changes in another branch).

@wagenet
Owner

@dgeb This seems ok. Not perfect but I don't have any better ideas.

@dgeb

@wagenet I agree with your assessment. Believe me, I'm not entirely happy with any of these solutions. I prefer the context/this change, but I understand the importance of handlebars compatibility. I guess I'll wait for direction from @wycats and @tomdale.

@wagenet
Owner

@dgeb What's the status on this?

@dgeb

@wagenet about a month ago, after checking with @tomdale and @wycats, I had just about wrapped up the context/this change:

dgeb@183f380

But after everyone's second thoughts about the change due to handlebars compatibility, this PR has gone cold. Unfortunately, I believe the original problem remains (I just checked the ViewHelper in master). It could still be solved with either the context/this change discussed above or with a new keyword to reference the view in the template.

Suggestions welcomed :)

@wagenet
Owner

I suggest a new keyword. I think breaking stock Handlebars/Mustache compat is a non-option.

@trek
Owner

@dgeb is this something you'd like to see in 1.0 or can we move this to 1.1 milestone?

@dgeb

@trek I think this should be resolved by 1.0 because it's a real inconsistency, even if it is rarely encountered. However, the question remains as to how...

All we need is the right keyword to reference a view from within its own {{view}} declaration. I've thrown around a few options in this ticket, but none have garnered much support (except the one suggestion (this) that breaks handlebars!).

Some options:

  • self
  • thisView
  • child
  • childView
  • inner
  • innerView

Or perhaps a symbol, such as a leading . ?

Anyway, I will gladly refresh this PR if an approach can be agreed upon.

@wagenet
Owner

@dgeb Since it's so rare, should we just tell people to make a custom view subclass?

@travisbot

This pull request fails (merged dd705c97 into 6ae344e).

@travisbot

This pull request passes (merged 39d9285 into 6ae344e).

@dgeb

In an effort to get this resolved by 1.0, I've updated this pull request to just fix the main inconsistency: class name bindings are now evaluated with the same context used to evaluate other bindings in a {{view}} helper.

I've punted on trying to add a new keyword to reference the view itself from within the {{view}} helper. As @wagenet suggests, people can simply use their own custom view subclasses for now. I've noted this in a couple places: in the new contextualizeBindingPath() method and in the {{collection}} helper test. Once this PR is merged, I could create a separate issue to track this.

@wagenet wagenet merged commit 01e26b0 into emberjs:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 10, 2012
  1. @dgeb

    {{view}} now evaluates the context of class bindings using the same r…

    dgeb authored
    …ules applied to other bindings
This page is out of date. Refresh to see the latest.
View
79 packages/ember-handlebars/lib/helpers/view.js
@@ -38,7 +38,8 @@ EmberHandlebars.ViewHelper = Ember.Object.create({
}
if (hash.classNameBindings) {
- extensions.classNameBindings = hash.classNameBindings.split(' ');
+ if (extensions.classNameBindings === undefined) extensions.classNameBindings = [];
+ extensions.classNameBindings = extensions.classNameBindings.concat(hash.classNameBindings.split(' '));
dup = true;
}
@@ -55,26 +56,57 @@ EmberHandlebars.ViewHelper = Ember.Object.create({
delete hash.classBinding;
}
- // Look for bindings passed to the helper and, if they are
- // local, make them relative to the current context instead of the
- // view.
- var path, normalized;
+ // Set the proper context for all bindings passed to the helper. This applies to regular attribute bindings
+ // as well as class name bindings. If the bindings are local, make them relative to the current context
+ // instead of the view.
+ var path;
+ // Evaluate the context of regular attribute bindings:
for (var prop in hash) {
if (!hash.hasOwnProperty(prop)) { continue; }
// Test if the property ends in "Binding"
if (Ember.IS_BINDING.test(prop) && typeof hash[prop] === 'string') {
- path = hash[prop];
-
- normalized = Ember.Handlebars.normalizePath(null, path, data);
- if (normalized.isKeyword) {
- hash[prop] = 'templateData.keywords.'+path;
- } else if (!Ember.isGlobalPath(path)) {
- if (path === 'this') {
- hash[prop] = 'bindingContext';
- } else {
- hash[prop] = 'bindingContext.'+path;
+ path = this.contextualizeBindingPath(hash[prop], data);
+ if (path) { hash[prop] = path; }
+ }
+ }
+
+ // Evaluate the context of class name bindings:
+ if (extensions.classNameBindings) {
+ var full,
+ parts;
+
+ for (var b in extensions.classNameBindings) {
+ full = extensions.classNameBindings[b];
+ if (typeof full === 'string') {
+ if (full.indexOf(':') > 0) {
+ // When a classNameBinding contains a colon anywhere after the first character,
+ // then the part preceding the colon is a binding path that needs to be
+ // contextualized.
+ //
+ // For example:
+ // classNameBinding="isGreen:green"
+ //
+ // Will be converted to:
+ // classNameBinding="bindingContext.isGreen:green"
+
+ parts = full.split(':');
+ path = this.contextualizeBindingPath(parts[0], data);
+ if (path) { extensions.classNameBindings[b] = path + ':' + parts[1]; }
+
+ } else if (full.indexOf(':') === -1 ) {
+ // When a classNameBinding doesn't contain any colons, then the entire binding
+ // needs to be contextualized.
+ //
+ // For example:
+ // classNameBinding="myClass"
+ //
+ // Will be converted to:
+ // classNameBinding="bindingContext.myClass"
+
+ path = this.contextualizeBindingPath(full, data);
+ if (path) { extensions.classNameBindings[b] = path; }
}
}
}
@@ -87,6 +119,23 @@ EmberHandlebars.ViewHelper = Ember.Object.create({
return Ember.$.extend(hash, extensions);
},
+ // Transform bindings from the current context to a context that can be evaluated within the view.
+ // Returns null if the path shouldn't be changed.
+ //
+ // TODO: consider the addition of a prefix that would allow this method to return `path`.
+ contextualizeBindingPath: function(path, data) {
+ var normalized = Ember.Handlebars.normalizePath(null, path, data);
+ if (normalized.isKeyword) {
+ return 'templateData.keywords.' + path;
+ } else if (Ember.isGlobalPath(path)) {
+ return null;
+ } else if (path === 'this') {
+ return 'bindingContext';
+ } else {
+ return 'bindingContext.' + path;
+ }
+ },
+
helper: function(thisContext, path, options) {
var inverse = options.inverse,
data = options.data,
View
115 packages/ember-handlebars/tests/handlebars_test.js
@@ -1144,41 +1144,121 @@ test("{{view}} should be able to point to a local view", function() {
equal(view.$().text(), "common", "tries to look up view name locally");
});
-test("should be able to bind view class names to properties", function() {
- var templates = Ember.Object.create({
- template: Ember.Handlebars.compile('{{#view "TemplateTests.classBindingView" classBinding="isDone"}}foo{{/view}}')
+test("{{view}} should evaluate class bindings set to global paths", function() {
+ Ember.run(function() {
+ window.App = Ember.Application.create({
+ isApp: true,
+ isGreat: true,
+ directClass: "app-direct"
+ });
});
- TemplateTests.classBindingView = Ember.View.extend({
- isDone: true
+ view = Ember.View.create({
+ template: Ember.Handlebars.compile('{{view Ember.TextField class="unbound" classBinding="App.isGreat:great App.directClass App.isApp"}}')
+ });
+
+ appendView();
+
+ ok(view.$('input').hasClass('unbound'), "sets unbound classes directly");
+ ok(view.$('input').hasClass('great'), "evaluates classes bound to global paths");
+ ok(view.$('input').hasClass('app-direct'), "evaluates classes bound directly to global paths");
+ ok(view.$('input').hasClass('is-app'), "evaluates classes bound directly to booleans in global paths - dasherizes and sets class when true");
+
+ Ember.run(function() {
+ App.set('isApp', false);
});
+ ok(!view.$('input').hasClass('is-app'), "evaluates classes bound directly to booleans in global paths - removes class when false");
+
+ Ember.run(function() {
+ window.App.destroy();
+ });
+});
+
+test("{{view}} should evaluate class bindings set in the current context", function() {
view = Ember.View.create({
- templateName: 'template',
- templates: templates
+ isView: true,
+ isEditable: true,
+ directClass: "view-direct",
+ template: Ember.Handlebars.compile('{{view Ember.TextField class="unbound" classBinding="isEditable:editable directClass isView"}}')
});
appendView();
- equal(view.$('.is-done').length, 1, "dasherizes property and sets class name");
+ ok(view.$('input').hasClass('unbound'), "sets unbound classes directly");
+ ok(view.$('input').hasClass('editable'), "evaluates classes bound in the current context");
+ ok(view.$('input').hasClass('view-direct'), "evaluates classes bound directly in the current context");
+ ok(view.$('input').hasClass('is-view'), "evaluates classes bound directly to booleans in the current context - dasherizes and sets class when true");
Ember.run(function() {
- set(firstChild(view), 'isDone', false);
+ view.set('isView', false);
});
- equal(view.$('.is-done').length, 0, "removes class name if bound property is set to false");
+ ok(!view.$('input').hasClass('is-view'), "evaluates classes bound directly to booleans in the current context - removes class when false");
});
-test("should be able to bind view class names to truthy properties", function() {
+test("{{view}} should evaluate class bindings set with either classBinding or classNameBindings", function() {
+ Ember.run(function() {
+ window.App = Ember.Application.create({
+ isGreat: true
+ });
+ });
+
+ view = Ember.View.create({
+ template: Ember.Handlebars.compile('{{view Ember.TextField class="unbound" classBinding="App.isGreat:great" classNameBindings="App.isGreat:really-great"}}')
+ });
+
+ appendView();
+
+ ok(view.$('input').hasClass('unbound'), "sets unbound classes directly");
+ ok(view.$('input').hasClass('great'), "evaluates classBinding");
+ ok(view.$('input').hasClass('really-great'), "evaluates classNameBinding");
+
+ Ember.run(function() {
+ window.App.destroy();
+ });
+});
+
+test("{{view}} should evaluate other attribute bindings set to global paths", function() {
+ Ember.run(function() {
+ window.App = Ember.Application.create({
+ name: "myApp"
+ });
+ });
+
+ view = Ember.View.create({
+ template: Ember.Handlebars.compile('{{view Ember.TextField valueBinding="App.name"}}')
+ });
+
+ appendView();
+
+ equal(view.$('input').attr('value'), "myApp", "evaluates attributes bound to global paths");
+
+ Ember.run(function() {
+ window.App.destroy();
+ });
+});
+
+test("{{view}} should evaluate other attributes bindings set in the current context", function() {
+ view = Ember.View.create({
+ name: "myView",
+ template: Ember.Handlebars.compile('{{view Ember.TextField valueBinding="name"}}')
+ });
+
+ appendView();
+
+ equal(view.$('input').attr('value'), "myView", "evaluates attributes bound in the current context");
+});
+
+test("{{view}} should be able to bind class names to truthy properties", function() {
var templates = Ember.Object.create({
template: Ember.Handlebars.compile('{{#view "TemplateTests.classBindingView" classBinding="number:is-truthy"}}foo{{/view}}')
});
- TemplateTests.classBindingView = Ember.View.extend({
- number: 5
- });
+ TemplateTests.classBindingView = Ember.View.extend();
view = Ember.View.create({
+ number: 5,
templateName: 'template',
templates: templates
});
@@ -1188,7 +1268,7 @@ test("should be able to bind view class names to truthy properties", function()
equal(view.$('.is-truthy').length, 1, "sets class name");
Ember.run(function() {
- set(firstChild(view), 'number', 0);
+ set(view, 'number', 0);
});
equal(view.$('.is-truthy').length, 0, "removes class name if bound property is set to falsey");
@@ -2004,7 +2084,7 @@ test("should not enter an infinite loop when binding an attribute in Handlebars"
expect(0);
Ember.run(function() {
- App = Ember.Application.create();
+ window.App = Ember.Application.create();
});
App.test = Ember.Object.create({ href: 'test' });
@@ -2035,8 +2115,7 @@ test("should not enter an infinite loop when binding an attribute in Handlebars"
});
Ember.run(function() {
- App.destroy();
- App = undefined;
+ window.App.destroy();
});
});
View
18 packages/ember-handlebars/tests/views/collection_view_test.js
@@ -248,26 +248,18 @@ test("should give its item views the classBinding specified by itemClassBinding"
});
var view = Ember.View.create({
- template: Ember.Handlebars.compile('{{#collection "TemplateTests.itemClassBindingTestCollectionView" itemClassBinding="content.isBaz"}}foo{{/collection}}')
+ isBar: true,
+ template: Ember.Handlebars.compile('{{#collection "TemplateTests.itemClassBindingTestCollectionView" itemClassBinding="isBar"}}foo{{/collection}}')
});
Ember.run(function() {
view.appendTo('#qunit-fixture');
});
- equal(view.$('ul li.is-baz').length, 2, "adds class on initial rendering");
+ equal(view.$('ul li.is-bar').length, 3, "adds class on initial rendering");
- Ember.run(function() {
- setPath(firstChild(view), 'content.0.isBaz', true);
- });
-
- equal(view.$('ul li.is-baz').length, 3, "adds class when property changes");
-
- Ember.run(function() {
- setPath(firstChild(view), 'content.0.isBaz', false);
- });
-
- equal(view.$('ul li.is-baz').length, 2, "removes class when property changes");
+ // NOTE: in order to bind an item's class to a property of the item itself (e.g. `isBaz` above), it will be necessary
+ // to introduce a new keyword that could be used from within `itemClassBinding`. For instance, `itemClassBinding="item.isBaz"`.
});
test("should give its item views the property specified by itemPropertyBinding", function() {
Something went wrong with that request. Please try again.