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.

Sign up for GitHub

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

Jump to bottom

feat(router): Allow navigation without updating the URL #9608

Merged
merged 1 commit into from Aug 4, 2016
Merged

feat(router): Allow navigation without updating the URL #9608

merged 1 commit into from Aug 4, 2016

Conversation

brandonroberts
Copy link
Contributor

@brandonroberts brandonroberts commented Jun 26, 2016 •
edited

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

The router always updates the URL on navigation

What is the new behavior?

The router can navigate without pushing a URL location update

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

Closes #9579 #9949

@brandonroberts brandonroberts force-pushed the router/silent-navigation branch 2 times, most recently from e74ae79 to a934b64 Compare June 26, 2016 03:17
@vicb vicb added feature Issue that requests a new feature area: router labels Jun 26, 2016
@vicb vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 26, 2016
pleerock
pleerock reviewed Jul 7, 2016
View reviewed changes
tools/public_api_guard/router/index.d.ts
@@ -88,8 +88,8 @@ export declare class Router {
routerState: RouterState;
url: string;
createUrlTree(commands: any[], {relativeTo, queryParams, fragment}?: NavigationExtras): UrlTree;
navigate(commands: any[], extras?: NavigationExtras): Promise<boolean>;
navigateByUrl(url: string | UrlTree): Promise<boolean>;
navigate(commands: any[], extras?: NavigationExtras, skipLocationChange?: boolean): Promise<boolean>;
Copy link

Choose a reason for hiding this comment

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

shouldn't skipLocationChange be a part of NavigationExtras ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion it shouldn't. NavigationExtras don't apply when using navigateByUrl because your building the URL manually and I'd prefer its use be consistent.

@pleerock
Copy link

pleerock commented Jul 7, 2016

this will not update a browser history, but still will emit events for this.activatedRoute.params, is it right?

@brandonroberts
Copy link
Contributor Author

Correct

@brandonroberts
Copy link
Contributor Author

Rebased

@ericmartinezr ericmartinezr mentioned this pull request Jul 14, 2016
Closed
3 tasks
@zoechi zoechi mentioned this pull request Jul 21, 2016
Closed
@brandonroberts
Copy link
Contributor Author

Rebased again. Will you review @vsavkin?

@lacolaco
Copy link
Contributor

Why is this suspended?

@damiandennis
Copy link

will this make it in rc.5?

vsavkin
vsavkin reviewed Jul 28, 2016
View reviewed changes
modules/@angular/router/src/router.ts
* ```
*
* In opposite to `navigateByUrl`, `navigate` always takes a delta
* that is applied to the current URL.
*/
navigate(commands: any[], extras: NavigationExtras = {}): Promise<boolean> {
return this.scheduleNavigation(this.createUrlTree(commands, extras), false);
navigate(commands: any[], extras: NavigationExtras = {}, skipLocationChange: boolean = false):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add skipLocationChange to extras?

@vsavkin vsavkin added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 28, 2016
@brandonroberts brandonroberts force-pushed the router/silent-navigation branch 2 times, most recently from 5d7f73c to f877c0f Compare July 29, 2016 11:46
@brandonroberts
Copy link
Contributor Author

Changes made @vsavkin

@brandonroberts
Copy link
Contributor Author

Rebased and merge conflicts fixed

@vsavkin vsavkin added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 4, 2016
@alxhub alxhub merged commit 63b82cd into angular:master Aug 4, 2016
@brandonroberts brandonroberts deleted the router/silent-navigation branch August 4, 2016 18:47
@AndreCAndersen AndreCAndersen mentioned this pull request Sep 25, 2017
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: router cla: yes feature Issue that requests a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to Navigate without pushing the location into the browser history
8 participants
@brandonroberts @pleerock @lacolaco @damiandennis @vsavkin @vicb @alxhub @googlebot