Skip to content
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.

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

Replace child parameter with builder on showDialog #15303

Merged
merged 9 commits into from Mar 14, 2018

Conversation

Projects
None yet
4 participants
@jonahwilliams
Copy link
Contributor

commented Mar 8, 2018

Changes the API of showDialog to take a Builder rather than any Widget. This change ensures that the dialog children are using the context from inside the dialog route, and not the context used to find the navigator.

This is a breaking change

If you require the BuildContext from the widget opening the dialog, you can always pass it through the Builder's closure.

Fixes #14341

@googlebot googlebot added the cla: yes label Mar 8, 2018

@mravn-google

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

I commented on the issue, but should have done it here, sorry: #14341 (comment)

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2018

Update builder parameter to take WidgetBuilder and make child argument deprecated

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2018

Also fixing #15194 since I am updating the showDialog documentation anyway

applicationLegalese: applicationLegalese,
children: children
);
}

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

trivial nit: indent on that '}' is off a bit

@@ -457,13 +457,18 @@ class _DialogRoute<T> extends PopupRoute<T> {

/// Displays a dialog above the current contents of the app.
///
/// This function typically receives a [Dialog] widget as its child argument.
/// Content below the dialog is dimmed with a [ModalBarrier].
/// This function typically receives a [Dialog] widget inside of a [WidgetBuilder]

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

not sure what you mean by "inside of a WidgetBuilder".

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Rephrased

/// as it's `builder` argument. Content below the dialog is dimmed with a [ModalBarrier].
/// This widget does not share a context with the location that `showDialog` is
/// originally called from, nor will calling `setState()` in this location cause
/// the dialog to update.

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

Maybe say "Use a [StatefulBuilder] or a custom [StatefulWidget] if the dialog needs to update dynamically."

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

@@ -481,12 +486,15 @@ class _DialogRoute<T> extends PopupRoute<T> {
Future<T> showDialog<T>({
@required BuildContext context,
bool barrierDismissible: true,
@required Widget child,
@deprecated Widget child,

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

@Deprecated('Instead of using the "child" argument, return the child from a closure provided to the "builder" argument. This will ensure that the BuildContext is appropriate for widgets built in the dialog.')

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

}) {
return Navigator.of(context, rootNavigator: true).push(new _DialogRoute<T>(
child: child,
child: child == null ? new Builder(builder: builder) : child,

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

child ?? new Builder(builder: builder)

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

@@ -481,12 +486,15 @@ class _DialogRoute<T> extends PopupRoute<T> {
Future<T> showDialog<T>({
@required BuildContext context,
bool barrierDismissible: true,
@required Widget child,
@deprecated Widget child,
@required WidgetBuilder builder,

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

builder can't be required while child is still present, since it's valid to only provide child in that case.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

@@ -481,12 +486,15 @@ class _DialogRoute<T> extends PopupRoute<T> {
Future<T> showDialog<T>({
@required BuildContext context,
bool barrierDismissible: true,
@required Widget child,
@deprecated Widget child,
@required WidgetBuilder builder,
}) {
return Navigator.of(context, rootNavigator: true).push(new _DialogRoute<T>(

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

assert that one of child and builder is non-null and that the other is null.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

barrierLabel: MaterialLocalizations.of(context).modalBarrierDismissLabel,
barrierLabel: MaterialLocalizations
.of(context)
.modalBarrierDismissLabel,

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

we would usually just put this on one line

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

height: 200.0,
),
)
builder: (BuildContext context) {

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 8, 2018

Contributor

nit: extraneous space

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 8, 2018

Author Contributor

Done

jonahwilliams added some commits Mar 8, 2018

@Deprecated(
'Instead of using the "child" argument, return the child from a closure '
'provided to the "builder" argument. This will ensure that the BuildContext'
' is appropriate for widgets built in the dialog.'

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 9, 2018

Contributor

trivial nit: convention is to put the space on the end of the line rather than the start

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 9, 2018

Author Contributor

Done

@@ -41,7 +41,7 @@ class UpdaterState extends State<Updater> {

final String updateUrl = await widget.updateUrlFetcher();
if (updateUrl != null) {
final bool wantsUpdate = await showDialog(context: context, child: _buildDialog());
final bool wantsUpdate = await showDialog(context: context, builder: (BuildContext context) => _buildDialog());

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 9, 2018

Contributor

best way to fix this one would be to make _buildDialog take a BuildContext context argument, then you can just pass that in directly.

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 9, 2018

Author Contributor

Named the parameter _ so that method would continue using the correct BuildContext

@Hixie

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

LGTM

/// This function typically receives a [Dialog] widget as its child argument.
/// Content below the dialog is dimmed with a [ModalBarrier].
/// This function typically receives a [WidgetBuilder] which build a [Dialog] widget
/// as it's `builder` argument. Content below the dialog is dimmed with a [ModalBarrier].

This comment has been minimized.

Copy link
@Hixie

Hixie Mar 9, 2018

Contributor

its

Though the way you phrased this it makes it sound like you mean the Dialog's builder argument. :-)

Maybe "This function takes a builder which typically builds a [Dialog] widget.`

This comment has been minimized.

Copy link
@jonahwilliams

jonahwilliams Mar 9, 2018

Author Contributor

Fixed!

jonahwilliams added some commits Mar 9, 2018

@jonahwilliams

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

Waiting until Monday for this.

@jonahwilliams jonahwilliams merged commit 231170a into flutter:master Mar 14, 2018

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
flutter-build

@jonahwilliams jonahwilliams deleted the jonahwilliams:dialog_updates_for_real branch Mar 14, 2018

DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018

Replace child parameter with builder on showDialog (flutter#15303)
* replace child parameter with builder on showDialog

* change builder parameter to WidgetBuilder

* mark child as deprecated and expand documentation to cover this and how builder should be used

* tidy comments and address some feedback

* phrasing

* move space to prev line and add //ignore comments for deprecated member use

* address comments and fix it's its

* update code samples

* adds semicolon to code snippets

mravn pushed a commit to mravn/flutter-wiki that referenced this pull request Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.