Skip to content

Pass RouteSettings to the internal Route in showCupertinoModalPopup #56024

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
Merged
goderbauer merged 8 commits into from
Oct 12, 2020
Merged

Pass RouteSettings to the internal Route in showCupertinoModalPopup #56024

goderbauer merged 8 commits into from
Oct 12, 2020

Conversation

@miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Apr 30, 2020 •
edited
Loading

Description

In this PR I add the missing RouteSettings parameter to the showCupertinoModalPopup method.

This parameter already existed in the internal _CupertinoModalPopupRoute but it was not exposed.

It is useful to expose this parameter to provide the route name to the newly created Route.

As example, the Material showDialog function accepts RouteSettings as well.

Related Issues

No related issues.

Tests

Added a test that obtains the route name from the RouteSettings passed to the showCupertinoModalPopup call and compares it to the expected value, to verify that the value is correctly passed.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.

@goderbauer goderbauer added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels May 8, 2020
@flutter-dashboard
Copy link

flutter-dashboard bot commented Sep 21, 2020

This pull request has not been updated in a while. Please update this pull request to receive results from Gold, or close it.

@miquelbeltran
Copy link
Member Author

miquelbeltran commented Sep 23, 2020

Hi @goderbauer, is the a change I can get a review? the code is not a huge change but helps to 1:1 the Cupertino and Material APIs

goderbauer
goderbauer reviewed Oct 1, 2020
View reviewed changes
packages/flutter/lib/src/cupertino/route.dart Outdated
ImageFilter filter,
bool useRootNavigator = true,
bool semanticsDismissible,
RouteSettings settings,
Copy link
Member

@goderbauer goderbauer Oct 1, 2020

Choose a reason for hiding this comment

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

To keep the naming consistent with the other showFoo methods, can we also call this here routeSettings?

goderbauer
goderbauer approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 12, 2020

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Oct 12, 2020

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

@goderbauer goderbauer merged commit da58f5e into flutter:master Oct 12, 2020
jonahwilliams pushed a commit that referenced this pull request Oct 12, 2020
Revert "Pass RouteSettings to the internal Route in showCupertinoModa…
cff9695 
…lPopup (#56024)"

This reverts commit da58f5e.
@jonahwilliams jonahwilliams mentioned this pull request Oct 12, 2020
Merged
jonahwilliams pushed a commit that referenced this pull request Oct 12, 2020
Revert "Pass RouteSettings to the internal Route in showCupertinoModa…
8aee75f 
…lPopup (#56024)" (#67969)

This reverts commit da58f5e.
@miquelbeltran miquelbeltran mentioned this pull request Oct 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@miquelbeltran @fluttergithubbot @goderbauer @googlebot