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

[BUGFIX release] Rerender link on routing state change. #11561

Merged
merged 1 commit into from
Jun 26, 2015

Conversation

juggy
Copy link
Contributor

@juggy juggy commented Jun 26, 2015

Properly update the route when the routing current state change. This primarily deals with query-params only routes as noted in issue #11533 .

@stefanpenner
Copy link
Member

can you add a failing test? I want to be sure i understand the actual issue (in code-form) this aims to work around.

@juggy
Copy link
Contributor Author

juggy commented Jun 26, 2015

Where are the tests for the link component? Can't seem to find them.

@stefanpenner
Copy link
Member

I don't know off the top of my head, but maybe they dont exist in isolation.

@juggy
Copy link
Contributor Author

juggy commented Jun 26, 2015

Thanks @mixonic. Here you go for the test. Failing on master but passing here.

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

Can you explain why the href computed dep key was not enough to update the href when the route changes? I am sure there is more internal detail that you tracked down when making this patch, so I'm hoping you can walk me through the 'why'...

@juggy
Copy link
Contributor Author

juggy commented Jun 26, 2015

There is a special case for query-params only link-tos as you can see here https://github.com/juggy/ember.js/blob/query-params-links/packages/ember-routing-views/lib/views/link.js#L452-L463

As you can see, the targetRouteName changes every time the route changes. The problem here is that the willRender is not called, because this change happens even if the attrs of the LinkComponent do not change (which would trigger a rerender).

Another solution to the problem would be to move this special case to the href computed property. That would remove the observer.

@juggy juggy force-pushed the query-params-links branch from 1cce287 to a8301e0 Compare June 26, 2015 19:00
Test link-to with only query params provided
@juggy juggy force-pushed the query-params-links branch from a8301e0 to e189688 Compare June 26, 2015 19:01
@juggy
Copy link
Contributor Author

juggy commented Jun 26, 2015

Removed the observer, moved the onlyQueryParamsSupplied logic to the href.

@stefanpenner
Copy link
Member

this appears much more reasonable :) Still needs someone else to review.

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

Looks good to me, can you prefix with [BUGFIX release]?

@juggy juggy changed the title Rerender link on routing state change. [BUGFIX release] Rerender link on routing state change. Jun 26, 2015
rwjblue added a commit that referenced this pull request Jun 26, 2015
[BUGFIX release] Rerender link on routing state change.
@rwjblue rwjblue merged commit dc479ce into emberjs:master Jun 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants