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

feat(router): add navigationSource and restoredState to NavigationStart event #21728

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@vsavkin
Copy link
Contributor

commented Jan 23, 2018

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

@googlebot googlebot added the cla: yes label Jan 23, 2018

@vsavkin vsavkin force-pushed the vsavkin:pass_navigation_id branch from ab077d4 to c77ffd9 Jan 23, 2018

@jasonaden jasonaden requested a review from IgorMinar Jan 23, 2018

@IgorMinar
Copy link
Member

left a comment

this looks very good, we just need to tweak the public api a bit.

@@ -208,6 +208,17 @@ export interface NavigationExtras {

/** @stable */
export declare class NavigationStart extends RouterEvent {
navigationSource?: NavigationSource;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 23, 2018

Member

NavigationSource is not exported but it's used as public api surface. is this intentional?

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 23, 2018

Member

I recommend inlining the string literal union into the public api + renaming the property to navigationTrigger (source sounds like the source URL, which this is not).

@@ -42,6 +47,26 @@ export class RouterEvent {
* @stable
*/
export class NavigationStart extends RouterEvent {
/** @docsNotRequired */

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 23, 2018

Member

why no docs?

/** @docsNotRequired */
navigationSource?: NavigationSource;

/** @docsNotRequired */

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 23, 2018

Member

why no docs?

@@ -208,6 +208,17 @@ export interface NavigationExtras {

/** @stable */
export declare class NavigationStart extends RouterEvent {
navigationSource?: NavigationSource;
restoredState?: {
navigationId: number;

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 23, 2018

Member

please add docs for the navigationId and what is its significance.

@vsavkin vsavkin force-pushed the vsavkin:pass_navigation_id branch from c77ffd9 to f3f144e Jan 24, 2018

@vsavkin

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2018

  • I added the missing docs.
  • I renamed NavigationSource into NavigationTrigger.
  • I exported NavigationTrigger instead of inlining it. Mainly cause it is already used in a handful of places in the router itself. Inlining will result in a lot of duplication.

@vsavkin vsavkin force-pushed the vsavkin:pass_navigation_id branch from f3f144e to 6c11dc0 Jan 24, 2018

@jasonaden jasonaden force-pushed the vsavkin:pass_navigation_id branch from 6c11dc0 to 6f83845 Jan 25, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

@jasonaden jasonaden force-pushed the vsavkin:pass_navigation_id branch from 6f83845 to af2c0b3 Jan 26, 2018

@jasonaden

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

I updated to remove the NavigationTrigger event from the external API. This is for documentation, so looking at this event on angular.io you will see the values you can use rather than the type name.

@IgorMinar
Copy link
Member

left a comment

this got submitted prematurely by accident. but all my comments were resolved so LGTM. thanks Victor.

jasonaden added a commit to jasonaden/angular that referenced this pull request Jan 27, 2018

Revert "feat(router): add navigationSource and restoredState to Navig…
…ationStart event (angular#21728)"

This reverts commit 3b7bab7. Will be re-merged after fixing integration of minor breaking change.

jasonaden added a commit to jasonaden/angular that referenced this pull request Jan 27, 2018

Revert: "feat(router): add navigationSource and restoredState to Navi…
…gationStart event (angular#21728)"

This reverts commit 3b7bab7. Will be re-merged after fixing integration of minor breaking change.

jasonaden added a commit to jasonaden/angular that referenced this pull request Jan 29, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

jbogarthyde added a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

jbogarthyde added a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

Revert: "feat(router): add navigationSource and restoredState to Navi…
…gationStart event (angular#21728)"

This reverts commit 3b7bab7. Will be re-merged after fixing integration of minor breaking change.

jbogarthyde added a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

Revert: "feat(router): add navigationSource and restoredState to Navi…
…gationStart event (angular#21728)"

This reverts commit 3b7bab7. Will be re-merged after fixing integration of minor breaking change.

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

Revert: "feat(router): add navigationSource and restoredState to Navi…
…gationStart event (angular#21728)"

This reverts commit 3b7bab7. Will be re-merged after fixing integration of minor breaking change.

jmleoni pushed a commit to jmleoni/angular that referenced this pull request Oct 6, 2018

feat(router): add navigationSource and restoredState to NavigationSta…
…rt event (angular#21728)

Currently, NavigationStart there is no way to know if an navigation was triggered imperatively or via the location change. These two use cases should be handled differently for a variety of use cases (e.g., scroll position restoration). This PR adds a navigation source field and restored navigation id (passed to navigations triggered by a URL change).

PR Close angular#21728
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.