fix(ion-tabs): preserve route navigation extras when changing tabs by jmannau · Pull Request #18493 · ionic-team/ionic-framework

jmannau · 2019-06-09T00:34:09Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

When using ion-tabs in an angular app, changing tabs doesn't restore the previous route navigation extras, such as Query Params or URL Fragments.

I haven't checked this behaviour in the react or vue distributions.

Issue Number: #18717

What is the new behavior?

This is a bug fix so that when changing tabs, any previous route navigation extras are restored.

Please note, this PR is only for @ionic/angular and not React or Vue projects.

Does this introduce a breaking change?

Yes
No

Other information

jmannau · 2019-07-26T00:40:13Z

Hi Ionic Team, you're doing an amazing job and I know are absolutely flat out. I'm just wondering if I can do anything else to help review this pull request so it can be approved and merged? Thanks a million!

liamdebeasi · 2019-08-07T12:49:33Z

Thanks for the PR! We appreciate all the work you put into this.

I think my main concern is changing what getLastUrl returns. This PR changes the return from a string to a RouteView which might be considered a breaking change for users that directly access IonRouterOutlet in their apps.

I am in favor of fixing this issue, but it might have to wait until the next major release of Ionic as we only do breaking changes in major releases. I will post in this thread when I have more to share.

Thanks!

jmannau · 2019-08-07T20:36:26Z

Thanks for the feedback!! Would it help if I updated the PR to include a new api that returns the RouteView, leaving getLastUrl to return a string? This would not introduce a breaking change and fix the bug at the same time. I’m happy to do this if you think it would work. Cheers, James

…

On 7 Aug 2019, at 10:50 pm, Liam DeBeasi ***@***.***> wrote: Thanks for the PR! We appreciate all the work you put into this. I think my main concern is changing what getLastUrl returns. This PR changes the return from a string to a RouteView which might be considered a breaking change for users that directly access IonRouterOutlet in their apps. I am in favor of fixing this issue, but it might have to wait until the next major release of Ionic as we only do breaking changes in major releases. I will post in this thread when I have more to share. Thanks! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

yannbf · 2019-08-14T12:31:20Z

Thanks for the PR! We appreciate all the work you put into this.

I think my main concern is changing what getLastUrl returns. This PR changes the return from a string to a RouteView which might be considered a breaking change for users that directly access IonRouterOutlet in their apps.

I am in favor of fixing this issue, but it might have to wait until the next major release of Ionic as we only do breaking changes in major releases. I will post in this thread when I have more to share.

Thanks!

@liamdebeasi what about #18740, which fixes the same problem however does not introduce a breaking change?

liamdebeasi · 2019-08-19T18:29:17Z

Sorry for the delay in replying!

What I'm thinking is we can add this change, but add it as a new method and update internal references to use this method. We would still have the old method, but we can mark it as deprecated. This will let us avoid a breaking change while still fixing the issue.

Reinstate getLastUrl to return a string for backwards compatability and add a new method getLastRouteView to return a RouteView complete with route navigation extras (query params and fragments)

jmannau · 2019-11-20T10:23:39Z

Hi @liamdebeasi I made time to look at this again and have restored getLastUrl to return a string for backward compatibility and added a new method getLastRouteView that returns the RouteView. This should remove any breaking changes. The ion-tab directive now uses getLastRouteView internally.

liamdebeasi · 2019-12-06T14:41:42Z

Thanks for making the changes! I found a small bug in the PR. Here are steps to reproduce:

Load Angular test app in src/angular/test/test-app in your browser.
Click "Tabs test".
Click "Go to Page 2 with Query Params".
Click "Tab Two". Notice that the query params/fragments disappear. (This is correct)
Click "Tab One". Notice that the query params/fragments re-appear. (This is correct)
Click the back button in the top left. Notice that the query params/fragments disappear. (This is correct)
Click "Go to Page 2 with Query Params".
Click "Tab Two".
Click "Tab One".
Click "Tab One" again. Notice that you go back a page as in step 6, but the query params/fragments do not disappear. (This part appears to be incorrect).

Other than that, this PR appears to be working properly.

jmannau · 2019-12-09T04:11:55Z

Hi @liamdebeasi I spent some today reviewing this carefully. I have updated the PR to take into account going to the root tab view and respecting the url extras. I have added documentation https://github.com/ionic-team/ionic/pull/18493/files#diff-891ce68da9d3204cb1a1870a225e5350R69 to describe different scenarios wrt tab navigation.

liamdebeasi

Great job!

liamdebeasi · 2019-12-11T15:49:00Z

PR has been merged. Thank you!

JeremyLopez · 2020-07-10T19:45:09Z

PR has been merged. Thank you!

Is there a version of @ionic/angular that contains this fix? I'm running 4.11.10 right now and selecting a new tab does not show this behavior. Or perhaps I'm missing something...

jmannau · 2020-07-10T20:35:12Z

The fix was only released with v5.0. Sorry.

…

On 11 Jul 2020, at 5:45 am, Jeremy Lopez ***@***.***> wrote: PR has been merged. Thank you! Is there a version of @ionic/angular that contains this fix? I'm running 4.11.10 right now and selecting a new tab does not show this behavior. Or perhaps I'm missing something... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

fix(ion-tabs): preserve route navigation extras when changing tabs

7310d35

ionitron-bot Bot added the package: angular @ionic/angular package label Jun 9, 2019

Merge branch 'master' into master

c454fdf

brandyscarney requested a review from liamdebeasi June 11, 2019 21:58

jmannau added 2 commits June 18, 2019 21:04

Merge branch 'master' into master

dded6bc

Merge branch 'master' into master

5b46896

yannbf mentioned this pull request Jul 18, 2019

bug: [Ionic v4] query params are getting lost when navigating between tabs #18717

Closed

liamdebeasi mentioned this pull request Aug 19, 2019

Bugfix/ion tabs query params #18740

Closed

13 tasks

jmannau added 3 commits November 20, 2019 20:03

Merge remote-tracking branch 'upstream/master'

debd092

(refactor) ion-tab preserve route params and fragments

90bbe67

Reinstate getLastUrl to return a string for backwards compatability and add a new method getLastRouteView to return a RouteView complete with route navigation extras (query params and fragments)

(chore) preseve formatting

6f4ed60

jmannau added 3 commits December 9, 2019 10:11

Merge remote-tracking branch 'upstream/master'

e10cd80

(fix) ion-tabs preserve url query params and fragments during navigation

fc88fa3

(chore) update ion-tabs tests

bbac599

mhartington reviewed Dec 10, 2019

View reviewed changes

Comment thread angular/src/directives/navigation/ion-router-outlet.ts

Comment thread angular/src/directives/navigation/ion-router-outlet.ts

Comment thread angular/src/directives/navigation/stack-controller.ts

jmannau added 2 commits December 11, 2019 10:38

Merge remote-tracking branch 'upstream/master'

071dc70

(chore) mark methods as @internal as per PR review

da69281

mhartington approved these changes Dec 11, 2019

View reviewed changes

Comment thread angular/src/directives/navigation/ion-router-outlet.ts

liamdebeasi approved these changes Dec 11, 2019

View reviewed changes

liamdebeasi merged commit 4c8f32f into ionic-team:master Dec 11, 2019

Uh oh!

Conversation

jmannau commented Jun 9, 2019 • edited by liamdebeasi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

jmannau commented Jul 26, 2019

Uh oh!

liamdebeasi commented Aug 7, 2019

Uh oh!

jmannau commented Aug 7, 2019 via email

Uh oh!

yannbf commented Aug 14, 2019

Uh oh!

liamdebeasi commented Aug 19, 2019

Uh oh!

jmannau commented Nov 20, 2019

Uh oh!

liamdebeasi commented Dec 6, 2019

Uh oh!

jmannau commented Dec 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

liamdebeasi left a comment

Choose a reason for hiding this comment

Uh oh!

liamdebeasi commented Dec 11, 2019

Uh oh!

JeremyLopez commented Jul 10, 2020

Uh oh!

jmannau commented Jul 10, 2020 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmannau commented Jun 9, 2019 •

edited by liamdebeasi

Loading