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

Support :: syntax in classNameBindings of Ember.View #732

Merged
merged 13 commits into from Jul 18, 2012

Conversation

Projects
None yet
10 participants
@pangratz
Member

pangratz commented Apr 24, 2012

This commit extends bindings to class names by adding support for falsy class names by using the double colon* syntax as following:

Ember.View.create({
  classNameBindings: ['isEnabled:enabled:disabled']
  isEnabled: true
});

This will add the class 'enabled' when isEnabled is true. If isEnabled is false the class 'disabled' is added instead.

*Inspired by recent semicolon discussion

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Apr 24, 2012

This pull request fails (merged b4c784b4 into 3c02542).

travisbot commented Apr 24, 2012

This pull request fails (merged b4c784b4 into 3c02542).

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Apr 24, 2012

Member

Seems like a useful concept. What do you think of the syntax isEnabled?enabled:disabled to show that it's acting as a ternary statement? I suppose isEnabled?enabled would also need to be supported for consistency.

Member

dgeb commented Apr 24, 2012

Seems like a useful concept. What do you think of the syntax isEnabled?enabled:disabled to show that it's acting as a ternary statement? I suppose isEnabled?enabled would also need to be supported for consistency.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Apr 24, 2012

Member

@dgeb Good point! I like your syntax better.

Member

pangratz commented Apr 24, 2012

@dgeb Good point! I like your syntax better.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Apr 24, 2012

Member

@pangratz This also needs to be added to the Handlebars helpers as well. However, we should make sure this is something we do want to add before you go to the trouble to add it there also.

Member

wagenet commented Apr 24, 2012

@pangratz This also needs to be added to the Handlebars helpers as well. However, we should make sure this is something we do want to add before you go to the trouble to add it there also.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Apr 24, 2012

Member

@wagenet thanks for the hint.

Member

pangratz commented Apr 24, 2012

@wagenet thanks for the hint.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Apr 24, 2012

Member

Just a note: there should be a single method which returns the class name for a property and this should be used in both places to DRY: binding.js and view.js

Member

pangratz commented Apr 24, 2012

Just a note: there should be a single method which returns the class name for a property and this should be used in both places to DRY: binding.js and view.js

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Apr 24, 2012

Member

@pangratz Would you like to DRY this up?

Member

wagenet commented Apr 24, 2012

@pangratz Would you like to DRY this up?

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Apr 24, 2012

Member

I'll give it a DRY. Uhh, Oscar for bad pun, here I come..

Member

pangratz commented Apr 24, 2012

I'll give it a DRY. Uhh, Oscar for bad pun, here I come..

@devinus

This comment has been minimized.

Show comment
Hide comment
@devinus

devinus Apr 26, 2012

Member

isFoo?foo:bar also has the unique advantage of being the JS ternary operator.

Member

devinus commented Apr 26, 2012

isFoo?foo:bar also has the unique advantage of being the JS ternary operator.

@devinus

This comment has been minimized.

Show comment
Hide comment
@devinus

devinus Apr 26, 2012

Member

Clever.

Member

devinus commented Apr 26, 2012

Clever.

@knassar

This comment has been minimized.

Show comment
Hide comment
@knassar

knassar Apr 26, 2012

I think the more flexible solution would be to allow attr/class bindings to a non-boolean computed property, with the return value of the property being used as the attribute value. Something like:

Ember.View.create({
  classNameBindings: ['elementClasses::'],
  isEnabled: true,
  elementClasses: Ember.computed(function() {
    if (this.get('isEnabled') === true) {
       return "enabled";
    } else if (this.get('isEnabled') === false) {
       return "disabled";
    } else {
       return "";
    }
  }).property('isEnabled')
});

knassar commented Apr 26, 2012

I think the more flexible solution would be to allow attr/class bindings to a non-boolean computed property, with the return value of the property being used as the attribute value. Something like:

Ember.View.create({
  classNameBindings: ['elementClasses::'],
  isEnabled: true,
  elementClasses: Ember.computed(function() {
    if (this.get('isEnabled') === true) {
       return "enabled";
    } else if (this.get('isEnabled') === false) {
       return "disabled";
    } else {
       return "";
    }
  }).property('isEnabled')
});
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Apr 26, 2012

Member

@knassar - Class names can currently be bound to computed properties. Here's an example I modified: http://jsfiddle.net/dgeb/hscQV/

I think the advantage of @pangratz's ternary solution is its brevity. Plus, it can be used from either the view class or the handlebars template (although #593 still needs to be sorted out).

Member

dgeb commented Apr 26, 2012

@knassar - Class names can currently be bound to computed properties. Here's an example I modified: http://jsfiddle.net/dgeb/hscQV/

I think the advantage of @pangratz's ternary solution is its brevity. Plus, it can be used from either the view class or the handlebars template (although #593 still needs to be sorted out).

@knassar

This comment has been minimized.

Show comment
Hide comment
@knassar

knassar Apr 26, 2012

Huh. I could have sworn I tried that and it didn't work. Thanks @dgeb

Well, in that case, I like the ternary operator version: property?trueClass:falseClass

knassar commented Apr 26, 2012

Huh. I could have sworn I tried that and it didn't work. Thanks @dgeb

Well, in that case, I like the ternary operator version: property?trueClass:falseClass

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 14, 2012

This pull request passes (merged 994dd8be into 4ef1568).

travisbot commented May 14, 2012

This pull request passes (merged 994dd8be into 4ef1568).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 14, 2012

This pull request passes (merged 9c600f24 into 4ef1568).

travisbot commented May 14, 2012

This pull request passes (merged 9c600f24 into 4ef1568).

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz May 14, 2012

Member

I rebased onto master and squashed the commits.

Member

pangratz commented May 14, 2012

I rebased onto master and squashed the commits.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 14, 2012

This pull request passes (merged c9cf4ab into 4ef1568).

travisbot commented May 14, 2012

This pull request passes (merged c9cf4ab into 4ef1568).

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb May 17, 2012

Member

Since this has a large amount of overlap with code I'm working on to finally wrap up #593, it would be nice to get this accepted sooner than later (of course, assuming it's acceptable :). Otherwise, either @pangratz or myself will end up doing a bunch of refactoring afterward.

Member

dgeb commented May 17, 2012

Since this has a large amount of overlap with code I'm working on to finally wrap up #593, it would be nice to get this accepted sooner than later (of course, assuming it's acceptable :). Otherwise, either @pangratz or myself will end up doing a bunch of refactoring afterward.

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Jun 2, 2012

Member

I like this, but I think we should colons.

Member

ebryn commented Jun 2, 2012

I like this, but I think we should colons.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 14, 2012

Member

I've rebased on to the latest master. The current implementation uses the ternary opertator syntax path?true-class:false-class. But changing it to double-colon syntax should be an easy change. So what do you think? Any comments on the implementation?

Member

pangratz commented Jul 14, 2012

I've rebased on to the latest master. The current implementation uses the ternary opertator syntax path?true-class:false-class. But changing it to double-colon syntax should be an easy change. So what do you think? Any comments on the implementation?

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 14, 2012

Member

It looks like I wasn't rebasing on to the latest master 😔

I'm on it ...

Member

pangratz commented Jul 14, 2012

It looks like I wasn't rebasing on to the latest master 😔

I'm on it ...

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jul 14, 2012

Member

@pangratz please take a look at how class bindings are now evaluated in the {{view}} helper. This will require some changes, regardless of which format (:: vs ?:) is chosen. I still like the ternary format, but I may be in the minority here.

Member

dgeb commented Jul 14, 2012

@pangratz please take a look at how class bindings are now evaluated in the {{view}} helper. This will require some changes, regardless of which format (:: vs ?:) is chosen. I still like the ternary format, but I may be in the minority here.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 14, 2012

Member

thanks @dgeb for the hint

Member

pangratz commented Jul 14, 2012

thanks @dgeb for the hint

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Jul 14, 2012

Member

If you make it use colon syntax, I will merge immediately :)

Member

ebryn commented Jul 14, 2012

If you make it use colon syntax, I will merge immediately :)

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 14, 2012

This pull request passes (merged b7aa61c into 3d8a3b3).

travisbot commented Jul 14, 2012

This pull request passes (merged b7aa61c into 3d8a3b3).

Change ternary syntax to double colon sytax
The syntax to define class names for truthy and falsy values changed from isEnabled?enabled:disabled to isEnabled:enabled:disabled
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 14, 2012

This pull request passes (merged b0d4845 into 3d8a3b3).

travisbot commented Jul 14, 2012

This pull request passes (merged b0d4845 into 3d8a3b3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 14, 2012

This pull request passes (merged 0e41ab5b into 3d8a3b3).

travisbot commented Jul 14, 2012

This pull request passes (merged 0e41ab5b into 3d8a3b3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 14, 2012

This pull request passes (merged 96840c6 into 3d8a3b3).

travisbot commented Jul 14, 2012

This pull request passes (merged 96840c6 into 3d8a3b3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 15, 2012

This pull request passes (merged 0d384ae into 3d8a3b3).

travisbot commented Jul 15, 2012

This pull request passes (merged 0d384ae into 3d8a3b3).

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 17, 2012

Member

Can I get some feedback on this?

Member

pangratz commented Jul 17, 2012

Can I get some feedback on this?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jul 18, 2012

Member

Seems good to me, @pangratz. I appreciate the refactoring you've done to consolidate property parsing and class name evaluation.

Member

dgeb commented Jul 18, 2012

Seems good to me, @pangratz. I appreciate the refactoring you've done to consolidate property parsing and class name evaluation.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 18, 2012

Member

Thanks @dgeb !

I was thinking about another thing: sometimes people ask for a binding where a class is bound if the value of the property is false. Previously you had to create a computed property or a Ember.Binding.not() for that:

Ember.View.extend({
   isNotEnabledBinding: Ember.Binding.not('isEnabled'),
   classNameBindings: ['isNotEnabled:not-enabled']
});

When this gets merged, it would be possible via:

Ember.View.extend({
   classNameBindings: ['isEnabled::not-enabled']
});

Is this something which should be supported or should I add an Ember.assert for the "true class" to not being empty?

Member

pangratz commented Jul 18, 2012

Thanks @dgeb !

I was thinking about another thing: sometimes people ask for a binding where a class is bound if the value of the property is false. Previously you had to create a computed property or a Ember.Binding.not() for that:

Ember.View.extend({
   isNotEnabledBinding: Ember.Binding.not('isEnabled'),
   classNameBindings: ['isNotEnabled:not-enabled']
});

When this gets merged, it would be possible via:

Ember.View.extend({
   classNameBindings: ['isEnabled::not-enabled']
});

Is this something which should be supported or should I add an Ember.assert for the "true class" to not being empty?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jul 18, 2012

Member

@pangratz I discussed this very question with @ebryn a few days ago and we agreed it's convenient to allow the true class to be empty so that isEnabled::not-enabled works (although I find the syntax a bit weird, especially coming from Ruby). So I'm glad this works - thanks for checking!

@ebryn Any more concerns or are you ready to merge?

Member

dgeb commented Jul 18, 2012

@pangratz I discussed this very question with @ebryn a few days ago and we agreed it's convenient to allow the true class to be empty so that isEnabled::not-enabled works (although I find the syntax a bit weird, especially coming from Ruby). So I'm glad this works - thanks for checking!

@ebryn Any more concerns or are you ready to merge?

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Jul 18, 2012

Member

@pangratz My intent was that isEnabled::not-enabled would work.

Member

ebryn commented Jul 18, 2012

@pangratz My intent was that isEnabled::not-enabled would work.

@devinus

This comment has been minimized.

Show comment
Hide comment
@devinus

devinus Jul 18, 2012

Member

Double colon is confusing, but I don't have a better suggestion.

Member

devinus commented Jul 18, 2012

Double colon is confusing, but I don't have a better suggestion.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 18, 2012

Member

Thanks for the feedback!

I'll add a test case covering the empty true class ...

Member

pangratz commented Jul 18, 2012

Thanks for the feedback!

I'll add a test case covering the empty true class ...

pangratz added some commits Jul 18, 2012

Add case where trueClass in classNameBindings is empty
So if a binding like

Ember.View.extend({
  classNameBindings: ['isEnabled::disabled']
});

adds no class if `isEnabled` is `true` and adds the class disabled when `isEnabled` is `false`
@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 18, 2012

Member

@ebryn I've added a test case for the empty true class case.

I'll do a rebase on master if this is good to go ...

Member

pangratz commented Jul 18, 2012

@ebryn I've added a test case for the empty true class case.

I'll do a rebase on master if this is good to go ...

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 18, 2012

This pull request passes (merged ae0dd6a into 3d8a3b3).

travisbot commented Jul 18, 2012

This pull request passes (merged ae0dd6a into 3d8a3b3).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 18, 2012

This pull request passes (merged 7df746d into 3d8a3b3).

travisbot commented Jul 18, 2012

This pull request passes (merged 7df746d into 3d8a3b3).

ebryn added a commit that referenced this pull request Jul 18, 2012

Merge pull request #732 from pangratz/add_double_colon_syntax
Support :: syntax in classNameBindings of Ember.View

@ebryn ebryn merged commit 0a4ece1 into emberjs:master Jul 18, 2012

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Jul 18, 2012

Member

Thanks @pangratz for adding this.

Member

ebryn commented Jul 18, 2012

Thanks @pangratz for adding this.

@pangratz

This comment has been minimized.

Show comment
Hide comment
@pangratz

pangratz Jul 18, 2012

Member

Wuhuuu, nice! 🤘

Thanks for your feedback!

Member

pangratz commented Jul 18, 2012

Wuhuuu, nice! 🤘

Thanks for your feedback!

@sly7-7

This comment has been minimized.

Show comment
Hide comment
@sly7-7

sly7-7 Jul 18, 2012

Contributor

Very nice, thx all of you for this feature.

Contributor

sly7-7 commented Jul 18, 2012

Very nice, thx all of you for this feature.

pangratz added a commit to pangratz/website that referenced this pull request Jul 19, 2012

@albertjan

This comment has been minimized.

Show comment
Hide comment
@albertjan

albertjan Aug 9, 2012

Awesome! thanks!

albertjan commented Aug 9, 2012

Awesome! thanks!

@sherwinyu

This comment has been minimized.

Show comment
Hide comment
@sherwinyu

sherwinyu Dec 19, 2012

Contributor

I'm just curious, what was the reason for preferring the double colon syntax over the ternary test?trueClass:falseClass syntax?

Contributor

sherwinyu commented Dec 19, 2012

I'm just curious, what was the reason for preferring the double colon syntax over the ternary test?trueClass:falseClass syntax?

@pangratz pangratz deleted the pangratz:add_double_colon_syntax branch Nov 14, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment