Skip to content
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

The classNameBindings property for the {{link-to}} helper doesn't update in 1.13.x #11699

Closed
courthead opened this issue Jul 8, 2015 · 8 comments · Fixed by #11960
Closed
Labels
Milestone

Comments

@courthead
Copy link

In Ember 1.12 you could do {{#link-to "path" classNameBindings="model.property:some-class"}}..., and when model.property changed the link's class would change accordingly. I'm finding that in Ember 1.13 the class is set initially, but doesn't update dynamically as model.property changes.

Here's a JSBin: http://emberjs.jsbin.com/duvocorodo/edit?html,css,js,output

@courthead courthead changed the title The classNameBindings property for the {{link-to}} helper doesn't update in 1.13 The classNameBindings property for the {{link-to}} helper doesn't update in 1.13.x Jul 8, 2015
@btecu
Copy link
Contributor

btecu commented Jul 9, 2015

Can confirm the same issue.

@mixonic
Copy link
Member

mixonic commented Jul 9, 2015

This seems like a regression. Confirmed on 1.13.3.

@mixonic mixonic added the Bug label Jul 9, 2015
@mixonic mixonic added this to the 1.13.x milestone Jul 9, 2015
@mutewinter
Copy link
Contributor

Confirmed, keeping us from migrating to 1.13.

@stefanpenner
Copy link
Member

if someone has time to submit a PR with a failing test, that would help get the ball moving.

@aortbals
Copy link
Contributor

I've submitted a failing test. I'm not entirely sure yet where in the stack the fix needs to be made. I'm looking into it. I'm happy to update the PR with the fix If I get to it before someone else. Any guidance on where this regression might have been introduced is also appreciated.

@raytiley
Copy link
Contributor

Not sure if its helpful but this also fails using the newer inline if syntax, which I imagine just uses attributeBindings under the hood?

I tried debugging this but got lost pretty quickly. In the ComponentNodeManager I see the attrs are correct but once block is called somewhere down the line the joinClasses helper gets called and the array of classes it gets has the old data. So all the changes / rerendering is happening, just somewhere down the line the updated attrs isn't getting passed along correctly.

edit html css js output 2015-07-15 08-14-15

@aortbals
Copy link
Contributor

Another few pieces of information:

After additional debugging, I wanted to confirm that this does seem to be specific to LinkComponent. When a link-to is rerendered with class bindings like the following, it does not update:

{{#link-to 'home' classNameBindings='isHomeHighlighted:is-highlighted'}}Home{{/link-to}}

And as @raytiley mentions, an inline if subexpression also does not work. Side note: I'm not sure which is correct here, class or classNames, both render once and do not rerender correctly like the previous example:

{{#link-to 'home' classNames=(if isHomeHighlighted 'is-highlighted')}}Home{{/link-to}}
{{#link-to 'home' class=(if isHomeHighlighted 'is-highlighted')}}Home{{/link-to}}

However, the following cases do rerender correctly:

Binding on a normal <a> tag

<a href='http://foo.bar' class={{if isHomeHighlighted 'is-highlighted'}}>Foo Bar</a>

A standard generated blank component with classNameBindings

{{#test-class-bindings classNameBindings='isHomeHighlighted:is-highlighted'}}Home{{/test-class-bindings}}

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2015

Nice summary of issues by @aortbals in #11391 (comment):

This doesn't appear to affect other components, only the LinkView component. See #11763. This seems to affect all bound attributes on link-to.

Issues: class #11390, #11763, tabindex #11763

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 a pull request may close this issue.

8 participants