packages icon indicating copy to clipboard operation
packages copied to clipboard

[go_router] Implement `popUntil`

Open ValentinVignal opened this issue 1 year ago • 10 comments

So I wanted to work on https://github.com/flutter/flutter/issues/107052 for pushAndRemoveUntil but it was closed saying it was a duplicate of https://github.com/flutter/flutter/issues/106402 which was not because it didn't contain the request for pushAndRemoveUntil. And https://github.com/flutter/flutter/issues/106402 got also closed because it got implemented.

So do you want me to re-create an issue? Or should we re-open https://github.com/flutter/flutter/issues/107052?


In any case, this PR first implements popUntil which is simpler than pushAndRemoveUntil. I will implement pushAndRemoveUntil if the implementation of this one is acceptable.

Pre-launch Checklist

  • [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • [x] I read the Tree Hygiene wiki page, which explains my responsibilities.
  • [x] I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • [x] I signed the CLA.
  • [x] The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • [x] I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is test-exempt.
  • [x] All existing and new tests are passing.

ValentinVignal avatar Oct 22 '22 13:10 ValentinVignal

Also relates to https://github.com/flutter/flutter/issues/99112

ValentinVignal avatar Oct 30 '22 11:10 ValentinVignal

Is there something else I need to do in this PR?

ValentinVignal avatar Oct 31 '22 15:10 ValentinVignal

I think these issues are closed due to we plan to use naive navigator API to handle imperative API. @johnpryan can chime in.

chunhtai avatar Nov 02 '22 17:11 chunhtai

Hello @chunhtai , @johnpryan , can I ask for a follow up on this?

Should I close this PR? If yes, is there a plan to make the delegate and route match list public? And can I help on that?

ValentinVignal avatar Nov 23 '22 15:11 ValentinVignal

Will this work for dialogs? I mean if I've open the dialog and then will call this popUntill, will predicate return the right value?

followthemoney1 avatar Jan 05 '23 09:01 followthemoney1

@chunhtai What's the status of this PR? It looks like it's probably waiting for another review.

stuartmorgan avatar Jan 17 '23 21:01 stuartmorgan

@stuartmorgan Actually, it requires some work from me first. https://github.com/flutter/packages/pull/2728#discussion_r1051403145 I need to find a way to make the delegate notify its listeners only once.

I was waiting for https://github.com/flutter/packages/pull/2846 to be merged. Because I personally have a bigger need for replace than popUntil in my projects so I was focusing my energy there

ValentinVignal avatar Jan 18 '23 01:01 ValentinVignal

Thanks, I'm going to go ahead and mark this as a draft then just to get it off of our review queue. Please set it back to Ready for Review once you've had a chance to update it!

stuartmorgan avatar Jan 31 '23 20:01 stuartmorgan

@ValentinVignal The https://github.com/flutter/packages/pull/2846 is about to be merged (just waiting for ci), will you work on this PR next?

chunhtai avatar Mar 16 '23 21:03 chunhtai

@ValentinVignal The #2846 is about to be merged (just waiting for ci), will you work on this PR next?

I'm not sure I'll work on it right away as I'm quite busy right now, but I'll work on it at some point when https://github.com/flutter/packages/pull/2846 is merged yes. But feel free to take it over if you want to

ValentinVignal avatar Mar 17 '23 01:03 ValentinVignal

Let me close this one for now, feel free to reopen once you have time.

chunhtai avatar Mar 23 '23 21:03 chunhtai

I'm still missing popUntil. It is irreplace-able feature that I'm missing. go_router should have it if it wants to be a full featured navigator. Some guy told me I could just use .go instead, but isn't .go like a push, won't it create another item in the navigation stack? I don't want that. I want to pop several pages. Use case is very simple. Page with a button, when the button is clicked navigates to a page with camera view, take photo, navigate to a full page preview pf the photo, click on confirm button, pop the preview page and the camera view. The only way to make this now is with 2 pops and have a special logic to get a value from the first pop. What if I have a wizard form with multiple pages. Having to implement a bunch of custom logic and calls of unknown number of pop.

talamaska avatar Apr 12 '23 17:04 talamaska

@talamaska go() is different from push(), go() replaces your current Navigator stack with a new one. For example, take this sample with two routes:

GoRoute(
  path: '/library',
  routes: [
    GoRoute(
      path: 'album/:albumId',
      routes: [
        GoRoute(
          path: 'song/:songId',
        ),
      ],
    ),
  ],
),

If you navigate to/library/album/1 by tapping on an item, the Navigator will have two Pages, one for /library and one for album/:albumId. If you tap the 'Library' button in the bottom navigation bar, the app will call go('/library') and the Navigator is rebuilt with only one Page, /library, so the second screen is popped from the Navigator and the correct transition animation is displayed, as if the user pressed the in-app back button.

johnpryan avatar Apr 12 '23 18:04 johnpryan