Skip to content

[FEATURE ember-routing-multi-current-when] link-to currentWhen can handle multiple routes#3673

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
gpoitch:current-when-multi
Jul 14, 2014
Merged

[FEATURE ember-routing-multi-current-when] link-to currentWhen can handle multiple routes#3673
rwjblue merged 1 commit intoemberjs:masterfrom
gpoitch:current-when-multi

Conversation

@gpoitch
Copy link
Copy Markdown
Contributor

@gpoitch gpoitch commented Oct 31, 2013

Expanding upon the link-to helper's currentWhen property to support multiple routes, using a pipe delimiter.

{{#link-to 'about' currentWhen='about|team'}}About{{/link-to}}

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Oct 31, 2013

@gdub22 - Could you update the docs (here) to reflect your changes?

Also, a single space seems like a more obvious separator choice to me, but lets wait to see what the core team folks think before changing it...

@stefanpenner
Copy link
Copy Markdown
Member

this would also be a feature, and should be marked and flagged as such. see: http://emberjs.com/guides/configuring-ember/feature-flags/

@wagenet
Copy link
Copy Markdown
Member

wagenet commented Nov 9, 2013

@gdub22 can you add the tag as @stefanpenner requested? We'll also need to discuss this with the core team.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Nov 11, 2013

Feature flagged

@gpoitch gpoitch closed this Nov 13, 2013
@gpoitch gpoitch deleted the current-when-multi branch November 13, 2013 18:53
@hyderali
Copy link
Copy Markdown
Contributor

hyderali commented Jan 3, 2014

@gdub22 Why was this closed ?

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Jan 3, 2014

@hyderali Accidentally got blown away during some housekeeping. Can redo if interested.

@hyderali
Copy link
Copy Markdown
Contributor

hyderali commented Jan 4, 2014

@gdub22 , Yeah, in need of this in my current project, and will be of great help, if this comes built-in.

@Kerrick
Copy link
Copy Markdown
Contributor

Kerrick commented Feb 7, 2014

Also would find incredibly useful, love it if this could be included in Ember.js core.

@selvagsz
Copy link
Copy Markdown

ping @gdub22 I believe this a useful addition to ember-core.

@gpoitch gpoitch reopened this Feb 13, 2014
@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Feb 13, 2014

@hyderali @Kerrick @selva-g recreated.

@selvagsz
Copy link
Copy Markdown

selvagsz commented Apr 4, 2014

Ping @wagenet @rjackson
Looks to be a long-pending thread. Any updates here ?

@wagenet
Copy link
Copy Markdown
Member

wagenet commented Apr 4, 2014

@gdub22 This needs to be rebased.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Apr 7, 2014

@wagenet @selvagsz rebased

@wagenet
Copy link
Copy Markdown
Member

wagenet commented Apr 8, 2014

@rjackson Did this get discussed in core?

@alexspeller
Copy link
Copy Markdown
Contributor

👍 This would be awesome

@GetContented
Copy link
Copy Markdown

There's something un-HTML-like about camelBack case: CSS sets the precedence for mulitple word attribute names, really. For example: word-wrap, or border-radius, etc. Also agree with @rjackson that space would be a better sub-string delimeter within attribute value (mulitple CSS classnames sets the precedence there).

@alexspeller
Copy link
Copy Markdown
Contributor

@JulianLeviston agreed, space is way more consistent with other things in ember whereas the pipe delimiter AFAIK is not used anywhere else.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Apr 9, 2014

I don't see how that CSS argument applies to this feature. My original thought is the pipe helps represent an 'or' operator, although it actually reads as a bitwise operator. One thing to note is that a route name with a space is actually valid. So is a pipe, but probably far less commonly used. Definitely all for consistency.

@alexspeller
Copy link
Copy Markdown
Contributor

@gdub22 yes I also don't see how the css argument applies. Routes are (generally) camelcased, and the currentWhen property accepting multiple routes doesn't apply here.

I didn't realize that routes with spaces in were valid. Perhaps they shouldn't be valid?

@GetContented
Copy link
Copy Markdown

The argument is that this is a template helper, so similar rules to HTML should apply, IMHO. BTW, no disrespect intended, gdub22. Your approach is very understandable and one can easily see why you chose what you chose.

@alexspeller
Copy link
Copy Markdown
Contributor

@JulianLeviston not sure if you mean the rules should apply to the name "currentWhen" or the arguments "fooRoute|barRoute", however neither makes sense - the name of the property is a javascript property that you are setting on the view and the arguments are route names. If you disagree about either of these naming conventions in ember this ticket is not the right place to argue for a change because those conventions apply throughout ember and handlebars, not just to this helper.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Apr 9, 2014

@JulianLeviston no worries :) currentWhen is actually already implemented in the ember api, this just expands upon it.

All unicode characters seem to be valid in route names.

@GetContented
Copy link
Copy Markdown

all good :)

@hyderali
Copy link
Copy Markdown
Contributor

hyderali commented May 5, 2014

@rjackson , any updates on this ?

@eniolopes
Copy link
Copy Markdown

I would love to be able to use this. 👍

@hyderali
Copy link
Copy Markdown
Contributor

@gdub22 Can you rebase this branch ?
@rjackson Has this been discussed in any of the core team meeting ?

@goto-bus-stop
Copy link
Copy Markdown

Just a hunch regarding the route list separator—

Handlebars.helper('list', function (...items, options) { return items; });
{{#link-to 'about' currentWhen=(list 'about' 'team')}}About{{/link-to}}

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jul 13, 2014

I'm generally 👍 on this. @gdub22 can you rebase? Will merge unless @machty disagrees, but it will still need to wait on +1/-1 from the rest of core before being enabled by default.

@machty
Copy link
Copy Markdown
Contributor

machty commented Jul 13, 2014

Happy to merge and deliberate while it remains behind a feature flag.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Jul 14, 2014

Working on rebasing... that code changed a lot.

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jul 14, 2014

@gdub22 - Yeah, the query-params stuff likely affected this area quite a bit. Sorry for taking so long in getting back to you, but thanks for working on this.

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Jul 14, 2014

@rjackson @hyderali
rebased.

rwjblue added a commit that referenced this pull request Jul 14, 2014
[FEATURE ember-routing-multi-current-when] link-to currentWhen can handle multiple routes
@rwjblue rwjblue merged commit 6bf8ff5 into emberjs:master Jul 14, 2014
@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Jul 14, 2014

Thanks!

@code-jongleur
Copy link
Copy Markdown

Following the EmberJS Change Log, the feature is included since EmberJS 1.6.0 release, right? I tried it. When I setup my .hbs template like in the first post {{#link-to 'about' currentWhen='about|team'}}About{{/link-to}}, I get an error message "Uncaught Error: There is no route named about|team". Ember doesn't seem to be able to resolve the routes correctly or to interpret them as two routes. Neither a pipe, a blank as a separator nor (list 'about' 'team') is working here. Did there anything change with the syntax in the meanwhile. I am a little bit confused. I am using EmberJS 1.6.1. Thank you very much for any hints!

@gpoitch
Copy link
Copy Markdown
Contributor Author

gpoitch commented Aug 5, 2014

@coachTomato This feature is not in 1.6.0. You have to use a canary build and enable the feature flag

@code-jongleur
Copy link
Copy Markdown

Ah, ok, I'll try that.

@tomdale
Copy link
Copy Markdown
Member

tomdale commented Aug 15, 2014

We discussed this at the core team meeting. We are go on the feature, but would like to change the syntax to be space-delimited instead of pipe-delimited.

@tomdale
Copy link
Copy Markdown
Member

tomdale commented Aug 15, 2014

@rwjblue will work on the patch this weekend when prepping the new releases.

@code-jongleur
Copy link
Copy Markdown

@tomdale: Great news. I have no problem to change the syntax to space-delimited in my code.

@Kerrick
Copy link
Copy Markdown
Contributor

Kerrick commented Aug 21, 2014

👍 Space-delimited seems more consistent with Ember's other features.

@trek
Copy link
Copy Markdown
Member

trek commented Aug 28, 2014

@gdub22 can you update based on @tomdale's feedback?

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Aug 28, 2014

@trek - This was already done, and is included in 1.8.0-beta.1 (with a space delimited list).

@rwjblue
Copy link
Copy Markdown
Member

rwjblue commented Aug 28, 2014

Was updated and enabled in #5387.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.