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

Helper to add css class based on active route #4387

Closed
manuelmitasch opened this Issue Feb 17, 2014 · 17 comments

Comments

Projects
None yet
8 participants
@manuelmitasch
Copy link
Contributor

manuelmitasch commented Feb 17, 2014

Issue

As discussed in #1822, #1849, and lately in manuelmitasch/ghost-admin-ember-demo@fba3ab0#commitcomment-5396470 there is currently no clean way to have an active css class (depending on active route) on a tag other than a link-to helper. This markup is used by bootstrap for example.

Desired output:

<li class="active>
  <a href="post">...</a>
</li>

Current Workaround

{{#link-to 'post' this tagName="li"}}
  {{#link-to 'post' this class="permalink" title="Edit this post"}}
    ...
  {{/link-to}}
{{/link-to}}

This double nesting is ugly.

Proposal

We should add an active-route helper, that adds an active class to the enclosing tag. The existing active route detection functionality of the Ember.LinkView should be moved into a mixin that can be reused for an Ember.ActiveRouteView.

Proposed usage

<li {{active-route 'post' this}}>
  {{#link-to 'post' this}}
    ...  
  {{/link-to}}
</li>

Other fancy usage

<body {{active-route view.classNames}}>

Sometimes, designers put classes on body to distinguish between styles for different templates. When the first parameter is no string (no route) it could lookup/add css classes from the handed property (eg. a views classNames property).

If you guys are happy with this proposal, I can try to implement a first draft.

PING @trek @stefanpenner

manuelmitasch referenced this issue in manuelmitasch/ghost-admin-ember-demo Feb 17, 2014

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Feb 17, 2014

Im not 100% on the proposal itself, but the problem is very clear. I would love to hear others thoughts?

@machty

This comment has been minimized.

Copy link
Member

machty commented Feb 18, 2014

We're very close to having this; might need to wait til HTMLbars though in the form of a sexpr though?

@manuelmitasch

This comment has been minimized.

Copy link
Contributor

manuelmitasch commented Feb 25, 2014

@machty is sexpr = subexpression? Is it the same concept as in handlebars (eg. new query params feature)? How could this be done with them?

@wagenet

This comment has been minimized.

Copy link
Member

wagenet commented Apr 2, 2014

@machty, @ebryn status?

@wagenet wagenet added the feature label Apr 2, 2014

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Apr 2, 2014

This isn't a builtin solution, but works pretty well: http://emberjs.jsbin.com/navew/2/edit

Implemented in Ghost's admin re-write here: TryGhost/Ghost#2541

In theory, the active checking stuff could be moved into a Mixin to allow easier usage (in custom components, views, handlebars helpers, etc).

@manuelmitasch

This comment has been minimized.

Copy link
Contributor

manuelmitasch commented Apr 3, 2014

@rjackson: I really like the neat little trick to create a binding between the active property of the component and alternateActive of LinkView. While it reads nicely in handlebars it still feels like a hack. The need to reopen Ember.LinkView and add a component just to add an active class seems a bit heavy.

What I noticed through your solution is that Ember.computed accepts dependent keys as optional first parameters. This is currently not documented. Is this intentional or can I add a pull request?

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Apr 3, 2014

@manuelmitasch - I agree. This is more difficult than it needs to be, it was just an illustration of how you could do this now (that doesn't completely suck). I'd prefer to not have to reopen LinkView.

I do not think that the presence of a custom component is an issue though. In your example you have to duplicate the route and args to the helper as well as the link-to that you would use. This is why I went with a custom component (so that the route name and args is only needed once in the template). To me this is EXACTLY what components are great for.

I have two specific PR's in mind for this:

  • Extract the active logic into a Mixin so that it is easy to use in a component directly without reopening LinkView.
  • Allowing the use to specify a custom view class to the link-to helper (this would allow us to implement our own custom LinkView with 'special' logic).
@ebryn

This comment has been minimized.

Copy link
Member

ebryn commented Apr 3, 2014

HTMLBars + subexpressions will make this a piece of cake.

@wagenet wagenet added the HTMLBars label Apr 12, 2014

@wagenet

This comment has been minimized.

Copy link
Member

wagenet commented Apr 12, 2014

See also #4707.

@Panman8201

This comment has been minimized.

Copy link
Contributor

Panman8201 commented Jul 10, 2014

FYI, Alex Speller posted a component that solves the Bootstrap issue. Very easy to use, less complex than this proposal.

http://discuss.emberjs.com/t/bootstrap-active-links-and-lis/5018

@wagenet

This comment has been minimized.

Copy link
Member

wagenet commented Aug 6, 2014

@machty ping.

@machty

This comment has been minimized.

Copy link
Member

machty commented Aug 6, 2014

I am close to a more fully realized solution to this.

@wagenet

This comment has been minimized.

Copy link
Member

wagenet commented Nov 1, 2014

@machty status?

@machty

This comment has been minimized.

Copy link
Member

machty commented Nov 1, 2014

Don't really have a status update; I think we're blocked on building a route service available to various controllers/components that can make calculating this sort of thing more easy, but we need to let the services PR settle.

@wagenet

This comment has been minimized.

Copy link
Member

wagenet commented Feb 28, 2015

@machty Services are in. Any further thoughts?

@chrisjstephens

This comment has been minimized.

Copy link

chrisjstephens commented Apr 28, 2015

Thanks Panman8201 for the link

That post seemed to work for me and wasn't too complicated.

@rwjblue

This comment has been minimized.

Copy link
Member

rwjblue commented Aug 9, 2015

@manuelmitasch - Thanks for the taking the time to submit this. As you can see we aren't terribly good at handling feature requests in the main repo's issues. Due to that, we have created the http://github.com/emberjs/rfcs repo to track proposals (as PR's) and requests for features (as issues). If you are still interested in pushing this forward, could you re-open this over there?

@rwjblue rwjblue closed this Aug 9, 2015

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