Use a parent class and View classNameBindings to set active states #231

tdreyno · 2012-07-19T18:57:20Z

Fixes #230.

Bind a class to the controller representing the filter state, add classes to each filter link, apply bold style based on the matching combination of these two classes.

Use a parent class and View classNameBindings to set active states

addyosmani · 2012-07-19T19:16:57Z

Thanks, @tdreyno!. I'm happy enough with this approach. Really appreciate you putting together the PR.

sindresorhus · 2012-07-20T00:24:30Z

Thanks :)

This method works here, but CSS and JS should generally be decoupled. What if it was like 20 filters. That wouldn't scale very well.

tdreyno · 2012-07-20T01:26:07Z

classNameBindings are a pretty integral part of Ember.View, so I'm not sure that statement is valid here. I get how it wouldn't scale if you were writing a ton of elem.className = in your JS, but this is something the framework supports and encourages.

sindresorhus · 2012-07-20T01:37:54Z

I'm just saying this doesn't scale:

.1-selected #filters li.1-filter a,
.2-selected #filters li.2-filter a,
.3-selected #filters li.3-filter a,
.4-selected #filters li.4-filter a,
.5-selected #filters li.5-filter a,
.6-selected #filters li.6-filter a,
.7-selected #filters li.7-filter a,
.8-selected #filters li.8-filter a,
.9-selected #filters li.9-filter a,
.10-selected #filters li.10-filter a,
.11-selected #filters li.11-filter a,
.12-selected #filters li.12-filter a,
.13-selected #filters li.13-filter a,
.14-selected #filters li.14-filter a,
.15-selected #filters li.15-filter a,
.16-selected #filters li.16-filter a,
.17-selected #filters li.17-filter a,
.18-selected #filters li.18-filter a,
.19-selected #filters li.19-filter a,
.20-selected #filters li.20-filter a {
    font-weight: bold;
}

What I don't like about it being tightly coupled with the CSS, is that each time you add a new filter, you have to update the CSS too.

I prefer:

#filters a.selected {
    font-weight: bold;
}

Short, generic and nicer. But I get that that isn't the Ember way, and I'm fine with that :)

tdreyno · 2012-07-20T01:50:20Z

Agreed. Though there's always Sass:

.selected { font-weight: bold; }

@for $n 1 through 20 {
  .#{$n}-selected #filters li.#{$n}-filter a { @extend .selected; }
}

tdreyno · 2012-07-20T02:02:45Z

Just in case anyone is curious what a view-based solution with .selected classes on their specific elements, something like:

FilterView = Em.View.extend({
  tagName: 'a',
  classNameBindings: 'selected'.w(),
  selected: function() {
    return this.get('filterBy') === Todos.entriesController.get('filterBy');
  }.property('Todos.entriesController.filterBy', 'filterBy')
});

<ul>
  <li>{{#view FilterView filterBy="all"}}All{{/view}}</li>
  <li>{{#view FilterView filterBy="active"}}Active{{/view}}</li>
  <li>{{#view FilterView filterBy="completed"}}Completed{{/view}}</li>
</ul>

addyosmani · 2012-07-20T09:38:48Z

Nice! Thanks for sharing the alternative take :)

On Friday, 20 July 2012, Thomas Reynolds wrote:

Just in case anyone is curious what a view-based solution with .selected
classes on their specific elements, something like:

FilterView = Em.View.extend({
  tagName: 'a',
  classNameBindings: 'selected'.w(),
  selected: function() {
    return this.get('filterBy') ===
Todos.entriesController.get('filterBy');
  }.property('Todos.entriesController.filterBy', 'filterBy')

<ul>
  <li>{{#view FilterView filterBy="all"}}All{{/view}}</li>
  <li>{{#view FilterView filterBy="active"}}Active{{/view}}</li>
  <li>{{#view FilterView filterBy="completed"}}Completed{{/view}}</li>
</ul>

Reply to this email directly or view it on GitHub:
#231 (comment)

Addy Osmani

Developer Programs Engineer at Google
Blogger at: http://addyosmani.com
Phone: +44 7501 594 382

sindresorhus · 2012-07-20T22:12:40Z

@tdreyno Sass way only works when using numbers, wouldn't work with words like we do in the app. It would also create a bloated CSS.

The alternative way is more like what i would have done. Is this any less of the Ember way?

tdreyno · 2012-07-20T23:11:51Z

Nope, it's actually more "Ember-y", I just didn't want to make things look too "Java-y" with tons of tiny classes everywhere. There's definitely a bit on tension between Handlebars "simplicity" and Ember.View's power.

Not sure if the binding on the entriesController would work with this repo's closure/namespacing. It seems like Todos is being passed around as app. Does that mean the global Todos might not exist or be renamed by a minifier?

I can send another PR with the second implementation if you'd prefer.

stas · 2012-07-20T23:52:59Z

architecture-examples/emberjs/js/views/application.js

@@ -58,6 +58,12 @@
 			}),
 			filtersView: Ember.View.create({
 				templateName: 'filtersTemplate',
+				classNameBindings: 'routerClassName'.w(),
+				currentFilterBinding: 'Todos.entriesController.filterBy',


This is not OK.
controller.namespace.entriesController.filterBy is probably a better way of doing this.

Anyway, if this works it's fine, but I'm not satisfied. It's just not that simplicity what Ember is supposed to offer.
Still waiting for current route state, somehow pragmatically, to be exposed to action.

Thanks @tdreyno for the patch! 👍

How would you do it differently if you had access to the current route state?

You actually have access to it: app.router.location.lastSetURL will give you /active if you're on the active route.

You are right, my mistake, Its more complex.

Anyway, If it were me, I would do it like this:

diff --git a/architecture-examples/emberjs/css/app.css b/architecture-examples/emberjs/css/app.css index 74b02a2..e1babc8 100644 --- a/architecture-examples/emberjs/css/app.css +++ b/architecture-examples/emberjs/css/app.css @@ -3,9 +3,3 @@ .hidden { display: none } - -.all-selected #filters li.all-filter a, -.active-selected #filters li.active-filter a, -.completed-selected #filters li.completed-filter a { - font-weight: bold; -} \ No newline at end of file diff --git a/architecture-examples/emberjs/index.html b/architecture-examples/emberjs/index.html index 1af33a2..10b116f 100755 --- a/architecture-examples/emberjs/index.html +++ b/architecture-examples/emberjs/index.html @@ -33,13 +33,13 @@ </script> <script id="filtersTemplate" type="text/x-handlebars"> <ul id="filters"> - <li class="all-filter"> - <a {{action showAll href=true}}>All </a> + <li {{action filterClicked target="view" on="click"}}> + <a {{action showAll href=true}}>All</a> </li> - <li class="active-filter"> - <a {{action showActive href=true}}>Active</a> + <li {{action filterClicked target="view" on="click"}}> + <a {{action showActive href=true on="click"}}>Active</a> </li> - <li class="completed-filter"> + <li {{action filterClicked target="view" on="click"}}> <a {{action showCompleted href=true}}>Completed</a> </li> </ul> diff --git a/architecture-examples/emberjs/js/views/application.js b/architecture-examples/emberjs/js/views/application.js index 6e8db4a..4088f84 100644 --- a/architecture-examples/emberjs/js/views/application.js +++ b/architecture-examples/emberjs/js/views/application.js @@ -58,12 +58,10 @@ }), filtersView: Ember.View.create({ templateName: 'filtersTemplate', - classNameBindings: 'routerClassName'.w(), - currentFilterBinding: 'Todos.entriesController.filterBy', - routerClassName: function() { - var currentFilter = this.get('currentFilter'); - return (currentFilter.length ? currentFilter : 'all') + '-selected'; - }.property( 'currentFilter' ) + filterClicked: function( event ) { + $( '#filters .selected' ).removeClass( 'selected' ); + $( event.target ).addClass('selected'); + } }), clearBtnView: Ember.View.create({ entriesBinding: 'controller.namespace.entriesController',

@stas But by doing this the selected state is coupled with the click even, which is wrong. Active should be selected on reload too. How would you solve that?

@sindresorhus you're right, dumb idea.
Whats bellow should be better:

diff --git a/architecture-examples/emberjs/css/app.css b/architecture-examples/emberjs/css/app.css index 74b02a2..e1babc8 100644 --- a/architecture-examples/emberjs/css/app.css +++ b/architecture-examples/emberjs/css/app.css @@ -3,9 +3,3 @@ .hidden { display: none } - -.all-selected #filters li.all-filter a, -.active-selected #filters li.active-filter a, -.completed-selected #filters li.completed-filter a { - font-weight: bold; -} \ No newline at end of file diff --git a/architecture-examples/emberjs/index.html b/architecture-examples/emberjs/index.html index 1af33a2..c73fd69 100755 --- a/architecture-examples/emberjs/index.html +++ b/architecture-examples/emberjs/index.html @@ -33,14 +33,14 @@ </script> <script id="filtersTemplate" type="text/x-handlebars"> <ul id="filters"> - <li class="all-filter"> - <a {{action showAll href=true}}>All </a> + <li> + <a {{action showAll href=true}} {{bindAttr class="view.isAll:selected"}}>All</a> </li> - <li class="active-filter"> - <a {{action showActive href=true}}>Active</a> + <li> + <a {{action showActive href=true}} {{bindAttr class="view.isActive:selected"}}>Active</a> </li> - <li class="completed-filter"> - <a {{action showCompleted href=true}}>Completed</a> + <li> + <a {{action showCompleted href=true}} {{bindAttr class="view.isCompleted:selected"}}>Completed</a> </li> </ul> </script> diff --git a/architecture-examples/emberjs/js/views/application.js b/architecture-examples/emberjs/js/views/application.js index 6e8db4a..4a5c96a 100644 --- a/architecture-examples/emberjs/js/views/application.js +++ b/architecture-examples/emberjs/js/views/application.js @@ -58,12 +58,16 @@ }), filtersView: Ember.View.create({ templateName: 'filtersTemplate', - classNameBindings: 'routerClassName'.w(), - currentFilterBinding: 'Todos.entriesController.filterBy', - routerClassName: function() { - var currentFilter = this.get('currentFilter'); - return (currentFilter.length ? currentFilter : 'all') + '-selected'; - }.property( 'currentFilter' ) + filterBinding: 'controller.namespace.entriesController.filterBy', + isAll: function( event ) { + return Ember.empty( this.get( 'filter' ) ) ? true : false; + }.property( 'filter' ), + isActive: function( event ) { + return this.get( 'filter' ) === 'active' ? true : false; + }.property( 'filter' ), + isCompleted: function( event ) { + return this.get( 'filter' ) === 'completed' ? true : false; + }.property( 'filter' ) }), clearBtnView: Ember.View.create({ entriesBinding: 'controller.namespace.entriesController',

I like this :)

@tdreyno How do you feel about this?

@stas You can shorten it to return !!this.get( 'filter' ) === 'completed' ;

Another question:

Is there a way to make this more generic, so that if I want another filter, a wouldn't have to create a new method on the filtersView?

Pseudo-code: <a {{action showCompleted href=true}} {{bindAttr class="view.isRoute(completed)"}}>Completed</a>

Had no idea about the :selected syntax on the classBindings. Pretty cool.

Looks great to me.

@sindresorhus can you apply the patch with your suggestions (also cleanup the event argument I carried over copycats)?
Regarding the question about being able to send params to bindAttr, afaik it's not possible :|

Thanks.

@stas Committed, thanks :)

In fact we could have used Ember.compare :)
Missed it, my bad.

https://github.com/emberjs/ember.js/blob/3b60016c2b6b38646d954014d10f74fcfe1a3d54/packages/ember-runtime/lib/core.js#L100-110

sindresorhus · 2012-07-21T10:12:59Z

Not sure if the binding on the entriesController would work with this repo's closure/namespacing. It seems like Todos is being passed around as app. Does that mean the global Todos might not exist or be renamed by a minifier?

Couldn't you use controller.namespace as @stas pointed out?

I can send another PR with the second implementation if you'd prefer.

I would like that. But I'll defer to @addyosmani to decide.

addyosmani · 2012-07-21T10:17:30Z

I certainly wouldn't mind reviewing a different take on this. At the end of the day the option we go for should be the one that most Ember developers would be more likely to use.

tdreyno · 2012-07-21T16:27:37Z

I think the anonymous closure and trying to pass around the namespace instead of having it global is complicating and confusing. Ember, and all it's docs, are pretty big on the whole global singleton approach. Maybe that's changing, what with controller being passed around everywhere recently.

sindresorhus · 2012-07-21T16:42:18Z

If @tomdale hasn't changed his view on it since the last time this was discussed, the recommended way is to hang everything on a global application instance.

Does that mean the global Todos might not exist or be renamed by a minifier?

Not unless you turn on the Advanced optimization flag when using Closure Compiler, and even then you could just do window['Todos'], so that shouldn't be a problem.

stas · 2012-07-21T17:16:32Z

@tdreyno regarding you question about namespace...
Scoping is very important when you want to test your component. Having an internal namespace that can be used to avoid calls to window makes your code maintainable and let's you split codebase in components (where you can describe an interface to every component, and assign teams to work on each one). Not to mention that during tests, you might end up never having a window['Todos'] instance.

Use a parent class and View classNameBindings to set active states

c3d9a2a

tdreyno mentioned this pull request Jul 19, 2012

Ember app doesn't update filter styles on change of route #230

Closed

addyosmani added a commit that referenced this pull request Jul 19, 2012

Merge pull request #231 from tdreyno/emberjs

9bb42cd

Use a parent class and View classNameBindings to set active states

addyosmani merged commit 9bb42cd into tastejs:emberjs Jul 19, 2012

sindresorhus mentioned this pull request Jul 20, 2012

Update emberjs example #63

Closed

stas reviewed Jul 20, 2012
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a parent class and View classNameBindings to set active states #231

Use a parent class and View classNameBindings to set active states #231

tdreyno commented Jul 19, 2012

addyosmani commented Jul 19, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

tdreyno commented Jul 20, 2012

addyosmani commented Jul 20, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

stas Jul 20, 2012

sindresorhus Jul 21, 2012

stas Jul 21, 2012

sindresorhus Jul 21, 2012

stas Jul 21, 2012

sindresorhus Jul 21, 2012

tdreyno Jul 21, 2012

stas Jul 21, 2012

sindresorhus Jul 22, 2012

stas Jul 22, 2012

sindresorhus commented Jul 21, 2012

addyosmani commented Jul 21, 2012

tdreyno commented Jul 21, 2012

sindresorhus commented Jul 21, 2012

stas commented Jul 21, 2012

Use a parent class and View classNameBindings to set active states #231

Use a parent class and View classNameBindings to set active states #231

Conversation

tdreyno commented Jul 19, 2012

addyosmani commented Jul 19, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

tdreyno commented Jul 20, 2012

addyosmani commented Jul 20, 2012

sindresorhus commented Jul 20, 2012

tdreyno commented Jul 20, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Jul 21, 2012

addyosmani commented Jul 21, 2012

tdreyno commented Jul 21, 2012

sindresorhus commented Jul 21, 2012

stas commented Jul 21, 2012