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] Fix link-to with only qps linking to outdated route #12058

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

cibernox
Copy link
Contributor

Fixes #12033

Once a link with only qps was created in a certain route, the targetRouteName was never updated again (the URL however was updated properly)

@@ -320,7 +320,7 @@ var LinkComponent = EmberComponent.extend({
}

var routing = get(this, '_routing');
var targetRouteName = get(this, 'targetRouteName');
var targetRouteName = this._handleOnlyQueryParamsSupplied(get(this, 'targetRouteName')); // Fixed version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. Can you flesh it out more or delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just forgot to delete it.

@cibernox cibernox force-pushed the fix_qps_only_links branch from aea059b to 296d359 Compare August 12, 2015 00:28
@cibernox cibernox force-pushed the fix_qps_only_links branch from 296d359 to 1b63604 Compare August 12, 2015 06:44
@deepflame
Copy link
Contributor

👍

experience the same bug. Great that there is a fix for it already.

@stefanpenner
Copy link
Member

@machty / @trek can you 👍 / 👎 as you are both fairly familiar with the QP code?

@@ -320,7 +320,7 @@ var LinkComponent = EmberComponent.extend({
}

var routing = get(this, '_routing');
var targetRouteName = get(this, 'targetRouteName');
var targetRouteName = this._handleOnlyQueryParamsSupplied(get(this, 'targetRouteName'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me but in hindsight, but in hindsight we should probably find a better name for this method... it doesn't seem to "handle" anything but rather compute the target route name? @juggy, you're the original author, would you be ok with renaming this method or is there something I'm missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but in hindsight, but in hindsight

does this reduce to "in the future " :trollface:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahahahah

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it can be renamed to something more meaningful like computeWithOnlyQueryParamsSupplied.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juggy - Mind sending in a PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #12065

@machty
Copy link
Contributor

machty commented Aug 12, 2015

Added a comment which might be beyond the scope of this PR but might be worth a quick fix if @juggy is quick enough to respond, else we can just merge.

rwjblue added a commit that referenced this pull request Aug 12, 2015
[BUGFIX release] Fix link-to with only qps linking to outdated route
@rwjblue rwjblue merged commit 4082280 into emberjs:master Aug 12, 2015
@cibernox cibernox deleted the fix_qps_only_links branch August 12, 2015 19:52
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.

6 participants