Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(ngRoute): add method for changing url params #8358

Closed

Conversation

NotBobTheBuilder
Copy link
Contributor

Request Type: feature

How to reproduce:

Component(s): ngRoute

Impact: medium

Complexity: small

This issue is related to:

Detailed Description:

Add a $route#update method for changing the current route
parameters without having to manually build a URL and call $location#path.
Useful for apps with a structure involving programmatically moving
between pages on the same route, but with different :param
values.

Other Comments:

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8358)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

Add a $route#update method for changing the current route
parameters without having to build a URL and call $location#path.
Useful for apps with a structure involving programmatically moving
between pages on the current route, but with different :param
values.
*
* @param {Object} newParams mapping of URL parameter names to values
*/
update: function(newParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call it updateParams?

@IgorMinar
Copy link
Contributor

this looks like a nice feature. please address the changes I requested. thanks!

This ensures that parameters are matched to the correct value when
being updated.
Adds a check to ensure the $location.path() changes to an expected
URL when the route changes.
Change the name of the method to make clearer its purpose.
Better to use angular methods than my clunky code.
@NotBobTheBuilder
Copy link
Contributor Author

Awesome; I pushed up a bunch of changes as you requested.

Hadn't seen angular.extend being used for more than one object before; makes total sense that it can be! And the less of my code needed the better :)

I accidentally added the updateParams tests inside of the tests for
`reloadOnSearch` - This commit extracts them to their own describe
block.
@NotBobTheBuilder
Copy link
Contributor Author

Moved the tests around (I put them under an unrelated test, they're in the right place now).

Not planning on making any changes past this point, unless you need me to :)

@jeffbcross jeffbcross self-assigned this Jul 28, 2014
@NotBobTheBuilder
Copy link
Contributor Author

Hate to be that guy - but any idea when this will get reviewed? :)

@petebacondarwin
Copy link
Contributor

@NotBobTheBuilder - at 11 days your PR is doing very well! We have merged some PRs after almost 12 months! Looks like it is in the milestone for the release this week so chances are it will be reviewed and merged soon

@NotBobTheBuilder
Copy link
Contributor Author

Cool!

I had no idea what to expect - there's loads of docs on filing PRs and code quality, but nothing that I can find on what happens after the ball leaves my court :)

On 6 Aug 2014, at 19:09, Pete Bacon Darwin notifications@github.com wrote:

@NotBobTheBuilder - at 11 days your PR is doing very well! We have merged some PRs after almost 12 months! Looks like it is in the milestone for the release this week so chances are it will be reviewed and merged soon


Reply to this email directly or view it on GitHub.

@jeffbcross
Copy link
Contributor

I'll be taking a look at this this week. One comment from @IgorMinar was that he's not sure the best way to handle updating query params with this API. One idea is to assume that property keys which aren't found in the path config should be set as query params.

@NotBobTheBuilder
Copy link
Contributor Author

I would agree; that's the default behaviour of $resource so it makes sense here too. I haven't tried doing it though, I'll write some tests for it.

@jeffbcross
Copy link
Contributor

I added a test and implemented the query param updating in #8601.

@jeffbcross
Copy link
Contributor

Merged 77a1acc

Thank you!

@jeffbcross jeffbcross closed this Aug 13, 2014
@NotBobTheBuilder NotBobTheBuilder deleted the ng-route-update-method branch August 13, 2014 21:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants