Skip to content

Conversation

ValentinVignal
Copy link
Contributor

This PR Adds void replace() and replaceNamed to GoRouterDelegate, GoRouter and GoRouterHelper.

It should fix flutter/flutter#106402

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@Musaddiq625
Copy link

@chunhtai please look into this

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

code looks good, left some documentation nits

@@ -70,6 +70,26 @@ extension GoRouterHelper on BuildContext {
extra: extra,
);

/// Replaces the current location with the given one w/ optional query
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Replaces the current location with the given one w/ optional query
/// Replaces the top-most page of page stack with the given URL location w/ optional query

@@ -70,6 +70,26 @@ extension GoRouterHelper on BuildContext {
extra: extra,
);

/// Replaces the current location with the given one w/ optional query
/// parameters, e.g. `/family/f2/person/p1?color=blue
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also mention go/push use

See also:

 * [go], which ....
 * [push], which ...

void replace(String location, {Object? extra}) =>
GoRouter.of(this).replace(location, extra: extra);

/// Replaces the current location with the named route w/ optional parameters,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Replaces the current location with the named route w/ optional parameters,
/// Replaces the top-most page of page stack with the named route w/ optional parameters,

GoRouter.of(this).replace(location, extra: extra);

/// Replaces the current location with the named route w/ optional parameters,
/// e.g. `name='person', params={'fid': 'f2', 'pid': 'p1'}`
Copy link
Contributor

Choose a reason for hiding this comment

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

also use the See also to mention goNamed and pushNamed

@ValentinVignal
Copy link
Contributor Author

Thank you @chunhtai, I have applied the changes in 33f7424

@ValentinVignal ValentinVignal requested a review from chunhtai July 19, 2022 02:10
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

@chunhtai
Copy link
Contributor

there is a merge conflict, can you resolve them before this pr can merge?

@chunhtai chunhtai requested a review from johnpryan July 19, 2022 17:27
# Conflicts:
#	packages/go_router/CHANGELOG.md
#	packages/go_router/pubspec.yaml
@ValentinVignal
Copy link
Contributor Author

there is a merge conflict, can you resolve them before this pr can merge?

I fixed the conflicts in a9019f9

@@ -1,3 +1,6 @@
## 4.2.0

- Adds `void replace()` and `replaceNamed` to `GoRouterDelegate`, `GoRouter` and `GoRouterHelper`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add an empty line between two versions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry for that, I added that in 8ec9776

@chunhtai
Copy link
Contributor

cc @johnpryan for a secondary review

@@ -78,6 +78,52 @@ void main() {
);
});

group('replace', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider adding a test for replaceNamed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for replaceNamed in 44e35ef Tell me what you think about it

@johnpryan johnpryan merged commit 199d8c9 into flutter:main Jul 22, 2022
johnpryan added a commit to johnpryan/flutter_packages that referenced this pull request Jul 22, 2022
johnpryan added a commit that referenced this pull request Jul 22, 2022
* Refactor internal classes and methods

- Separate matching from redirection
- Add RouteRedirector typedef
- Add RouteMatcher class
- Add RouteBuilder class
- Add RouteConfiguration class
- Rename and reorganize internal classes and libraries
- Add todo comments

* format

* Sort imports

* Update changelog

* Address code review comments

- Change name back to GoRouterRefreshStream
- Update toString() methods for new naming
- Make fields final
- Add logging to parser
- Add comments
- add tests
- Move function-scope to new library-scope _addRedirect function
- import widgets instead of material where possible

* remove routing library

* Move classes in go_router.dart into separate libraries

* Move Configuration.validate() into constructor

* Remove comment

* use continue in redirect loop

* Fix comments

* Sort imports

* Fix logging in configuration

* add visibleForTesting annotation

* Updates from merge with main

* Format

* Add TODOs to make Router implementation classes private

* Add copyright headers

* Fix tests

* format

* fix comment

* Update packages/go_router/lib/src/parser.dart

Co-authored-by: Loïc Sharma <[email protected]>

* add whitespace

* format

* Hide typedefs that weren't previously exported

* Delete empty file

* add missing import

* Specify version 4.1.2 in pubspec.yaml

* Update packages/go_router/lib/src/builder.dart

Co-authored-by: chunhtai <[email protected]>

* Fix comment

* Add isError and error getters to RouteMatchList

* Add issue links to TODO comments

* Add link to issue for TODO

* Re-apply code from #2306 due to merge conflicts

* Add issue references

Co-authored-by: Loïc Sharma <[email protected]>
Co-authored-by: chunhtai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[go_route ] replacement & replacementNamed
4 participants