Add "navigator" option to "showDialog" and "showGeneralDialog" by brianegan · Pull Request #42842 · flutter/flutter

brianegan · 2019-10-16T12:42:12Z

Description

The problem: We need to use showDialog to push a dialog to sub Navigator, not the root Navigator. At the moment, showDialog uses showGenericDialog which always uses the root navigator.

The solution: This Pull Request adds the ability to specify the NavigatorState which should be used to push dialog routes.

Alternative solution: Accept an optional bool useRootNavigator = true parameter instead of a NavigatorState.

Related Issues

Allows Next 10 Diagrams! assets-for-api-docs#106 to generate diagrams properly
Fixes openDialog / openGeneralDialog should not force use of root Navigator #23565

Tests

I added the following tests:

Before writing tests I wanted feedback on the general idea. Tests will be added after completing any necessary api discussions.

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 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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

fluttergithubbot · 2019-10-16T12:42:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

goderbauer

Approach seems OK. This will need a test, though.

jimmy-robert

What about other showXXX() functions that also use Navigator or showDialog?

showAboutDialog
showCupertinoDialog
showCupertinoModalPopup
showLicensePage
showMenu
showModalBottomSheet
showSearch
showTimePicker

Piinks · 2019-10-25T19:19:10Z

@brianegan Are you planning on revisiting this?

brianegan · 2019-10-29T09:56:09Z

@Piinks Yep! Sorry, had another bit of work to wrap up the past week or so.

After seeing @JimmyRobz comment, I realized this is actually a pretty decent sized change to quite a few methods. Since the assets-for-api-docs project is quite unique in how it works and is kind of "non-standard", I wanted to take a bit more time to see if I could get around the issue without changing so many methods in Flutter itself.

After spending a few more hours on that, I think I feel pretty confident in saying the only way to fix the issue there is to make this change!

brianegan · 2019-10-29T10:03:53Z

@JimmyRobz @goderbauer Thanks for your feedback! I have a question I'd love to ask: Is a NavigatorState navigator argument better than a bool useRootNavigator argument?

There were two ideas for this change -- allow this function to accept a bool useRootNavigator argument, or allow these functions to accept a specific NavigatorState navigator. @goderbauer suggested we go with the NavigatorState option, but while going through each showX method, I noticed the showModalBottomSheet already has a useRootNavigator option.

useRootNavigator

Pros:

API consistency with what's already in place
No need to remove @required annotation from context on several methods. Overall, I worry about removing that annotation and replacing it with an assert, since the annotation provides IDE completion / warnings whereas asserts only fail at runtime.

Cons:

Can't specify the exact navigator you want to use. Need to rely on the nearest Navigator.

NavigatorState

Pros:

Specify the exact NavigatorState you want to use

Cons:

Requires removing @required annotation
Maybe confusing to pass either a context or navigator into the function? Some methods would require both to get the desired effect.

goderbauer · 2019-10-29T18:17:15Z

Since we already have useRootNavigator on one of them and we don't really have a user case for anything else I'd be fine with just adding useRootNavigator.

brianegan · 2019-10-30T17:05:03Z

Thanks @goderbauer -- I've now changed everything to useRootNavigator. I've also added 1 test to the showDialog test suite. Could you please verify that's the correct / best way to test this change? If it looks ok or you have improvements, I can apply a similar test to all functions.

goderbauer

Looks good. Just some nits.

goderbauer · 2019-10-30T17:16:17Z

This should mention what the default is (here and elsewhere).

You may want to use a {@macro ...} for this.

Mentioned the default for every function.

I decided against a macro for now since there isn't much duplication and I find macro docs quite inconvenient as a user (since it forces you to leave your editor and head to the api docs website in order to view the information).

Let me know if you feel strongly about using @macro, though -- can be easily changed.

- Tests for each function - Mention `useRootNavigator` default in every function's docs - Assert root navigator is not null in every function - Ensure correct default values for every function

brianegan · 2019-10-31T14:31:41Z

Thanks again, @goderbauer. I've added tests to each function and addressed all feedback.

- Mention useRootNavigator default in every function's docs
- Assert root navigator is not null in every function
- Ensure correct default values for every function

goderbauer

LGTM

brianegan · 2019-11-01T11:26:29Z

Thanks, @goderbauer! Should I hold off on merging this change since flutter-build is currently broken on this branch? Is there anything I can do to fix it or is the problem unrelated to my changes?

goderbauer · 2019-11-01T17:20:31Z

Yeah, you'll have to wait until all checks are green. The current redness is unrelated to your change.

…er#42842)

fluttergithubbot added the p: material_ui material_ui package in flutter/packages label Oct 16, 2019

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 16, 2019

googlebot added the cla: yes label Oct 16, 2019

brianegan changed the title ~~Add navigator option to showDialog and showGeneralDialog~~ Add "navigator" option to "showDialog" and "showGeneralDialog" Oct 16, 2019

goderbauer reviewed Oct 16, 2019

View reviewed changes

Comment thread packages/flutter/lib/src/material/dialog.dart Resolved

Comment thread packages/flutter/lib/src/material/date_picker.dart Resolved

Comment thread packages/flutter/lib/src/widgets/routes.dart Resolved

Comment thread packages/flutter/lib/src/widgets/routes.dart Resolved

Piinks added c: new feature Nothing broken; request for a new capability f: routes Navigator, Router, and related APIs. labels Oct 17, 2019

jimmy-robert suggested changes Oct 19, 2019

View reviewed changes

jimmy-robert mentioned this pull request Oct 19, 2019

Dialog widget is out of phone boundaries aloisdeniel/flutter_device_preview#7

Closed

Piinks removed the p: material_ui material_ui package in flutter/packages label Oct 25, 2019

brianegan force-pushed the dialog-navigator branch from ab6fbeb to 7e9de1e Compare October 30, 2019 17:03

goderbauer reviewed Oct 30, 2019

View reviewed changes

brianegan added 4 commits October 31, 2019 15:19

Add navigator option to showDialog and showGeneralDialog

e0b2b7f

Add navigator to showDatePicker

4c6078b

Move from navigator argument to useRootNavigator

92c81b8

Add tests for every function, update based on feedback

9a43dd0

- Tests for each function - Mention `useRootNavigator` default in every function's docs - Assert root navigator is not null in every function - Ensure correct default values for every function

brianegan force-pushed the dialog-navigator branch from 7e9de1e to 9a43dd0 Compare October 31, 2019 14:19

Fix documentation

67720bd

goderbauer approved these changes Oct 31, 2019

View reviewed changes

goderbauer added the waiting for tree to go green label Nov 1, 2019

Piinks merged commit 2e41483 into flutter:master Nov 1, 2019

Piinks mentioned this pull request Nov 4, 2019

Next 10 Diagrams! flutter/assets-for-api-docs#106

Merged

brianegan deleted the dialog-navigator branch November 5, 2019 10:51

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

Add "navigator" option to "showDialog" and "showGeneralDialog" (flutt…

407a3e1

…er#42842)

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

brianegan commented Oct 16, 2019 • edited by Piinks Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

fluttergithubbot commented Oct 16, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimmy-robert left a comment

Choose a reason for hiding this comment

Uh oh!

Piinks commented Oct 25, 2019

Uh oh!

brianegan commented Oct 29, 2019

Uh oh!

brianegan commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

useRootNavigator

NavigatorState

Uh oh!

goderbauer commented Oct 29, 2019

Uh oh!

brianegan commented Oct 30, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

goderbauer Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

goderbauer Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

brianegan Oct 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brianegan commented Oct 31, 2019

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

brianegan commented Nov 1, 2019

Uh oh!

goderbauer commented Nov 1, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

brianegan commented Oct 16, 2019 •

edited by Piinks

Loading

brianegan commented Oct 29, 2019 •

edited

Loading