Adaptive alert dialog #124336

MitchellGoodwin · 2023-04-06T17:44:16Z

Fixes #102811. Adds an adaptive constructor to AlertDialog, along with the adaptive function showAdaptiveDialog.

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 Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
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.

MitchellGoodwin · 2023-04-06T19:34:09Z

packages/flutter/lib/src/material/dialog.dart

+///
+/// On Cupertino platforms, [barrierColor], [useSafeArea], and
+/// [traversalEdgeBehavior] are ignored.
+Future<T?> showAdaptiveDialog<T>({


Do we have a pattern of adaptive functions? I wasn't sure the right way to name this.

This name sounds good to me. I don't think we have any patterns for the name of the adaptive function. But I'm not very sure. Maybe we can collect some suggestions from @HansMuller ;)

@HansMuller if you could take a quick peek at this.

QuncCccccc

LGTM:) !

QuncCccccc · 2023-04-07T00:55:16Z

examples/api/lib/material/dialog/adaptive_alert_dialog.0.dart

+  Widget build(BuildContext context) {
+    return MaterialApp(
+      // Try this: set the platform to platform.android and see the difference
+      theme: ThemeData(platform: TargetPlatform.iOS),


Maybe here we can also set useMaterial3 to true to see the latest style of Material alert dialog:)

QuncCccccc · 2023-04-07T00:57:04Z

packages/flutter/lib/src/material/dialog.dart

+///
+/// On Cupertino platforms, [barrierColor], [useSafeArea], and
+/// [traversalEdgeBehavior] are ignored.
+Future<T?> showAdaptiveDialog<T>({


This name sounds good to me. I don't think we have any patterns for the name of the adaptive function. But I'm not very sure. Maybe we can collect some suggestions from @HansMuller ;)

QuncCccccc · 2023-04-07T00:58:50Z

packages/flutter/test/material/dialog_test.dart

+      ),
+      actions: <Widget>[
+        TextButton(
+            onPressed: () {},


nit: maybe two space indent.

examples/api/lib/material/dialog/adaptive_alert_dialog.0.dart

werainkhatri · 2023-04-08T13:18:31Z

packages/flutter/lib/src/material/dialog.dart

@@ -24,6 +24,8 @@ import 'theme_data.dart';

 const EdgeInsets _defaultInsetPadding = EdgeInsets.symmetric(horizontal: 40.0, vertical: 24.0);

+enum _DialogType {material, adaptive }


nit:

Suggested change

enum _DialogType {material, adaptive }

enum _DialogType { material, adaptive }

packages/flutter/test/material/dialog_test.dart

packages/flutter/lib/src/material/dialog.dart

werainkhatri · 2023-04-10T18:52:26Z

packages/flutter/lib/src/material/dialog.dart

+  /// are ignored: [title], [content], [actions], [scrollController],
+  /// [actionScrollController], [insetAnimationDuration], and
+  /// [insetAnimationCurve]. If a [AlertDialog] is created, [scrollController],


the newly added params are NOT ignored in case of CupertinoAlertDialog. they're only ignored in case of material AlertDialog.

werainkhatri · 2023-04-10T19:21:04Z

packages/flutter/lib/src/material/dialog.dart

+  /// Typically passed as a child of [showAdaptiveDialog], which will display
+  /// the alert differently based on platform.
+  ///
+  /// If a [CupertinoAlertDialog] is created, all parameters are ignored except


i'm sorry, but i feel stating it as a positive instead of a double negative would be easier to understand, something like ... only these parameters are used:. could you check other adaptive widgets / functions for similar documentation?

moving the statement for AlertDialog to a newline would help too.

lemme know your thoughts.

No, that makes sense. On the other adaptive widgets we so far have only called out which ones are ignored, instead of which ones aren't.

QuncCccccc

It looks good! Just left one comment for the newly added fields:) Thanks!

QuncCccccc · 2023-04-18T16:51:19Z

packages/flutter/lib/src/material/dialog.dart

@@ -634,10 +703,73 @@ class AlertDialog extends StatelessWidget {
  /// button bar.
  final bool scrollable;

+  /// A scroll controller that can be used to control the scrolling of the


nit: Maybe we can add {@template ...}... {@endtemplate} for the cupertino AlertDialog's API documentation so that here we can directly use {@macro ...}

Question: would this be necessary if we go with the factory implementation you mentioned below? That would hide the Cupertino specific properties somewhat. I can add them with the documentation for the .adaptive but I'm thinking it's better to just let them go to the page for CupertinoAlertDialog at that point.

Ah that makes sense. SGTM!

QuncCccccc · 2023-04-18T17:10:54Z

packages/flutter/lib/src/material/dialog.dart

+  ///
+  ///  * [actionScrollController], which can be used for controlling the actions
+  ///    section when there are many actions.
+  final ScrollController? scrollController;


I'm considering if we should add these 4 APIs here because the material AlertDialog will not use them at all. Another way I can think of is to use factory keyword when we create AlertDialog.adaptive constructor, and the factory constructor will be assigned to a private subclass which has the extra APIs. An example can be FilledButton.icon(). By using factory, we can add new parameters only in the AlertDialog.adaptive constructor without adding new fields for AlertDialog. I think that would be something like: https://gist.github.com/QuncCccccc/93f22070b6b459912eeddb03ff55d1ec

But please let me know if this doesn't make sense:)

No that makes sense, and that looks a lot cleaner. Thank you!

that's way better.

QuncCccccc · 2023-04-18T19:44:29Z

examples/api/lib/material/dialog/adaptive_alert_dialog.0.dart

+import 'package:flutter/cupertino.dart';
+import 'package:flutter/material.dart';
+
+void main() => runApp(const MyApp());


Ah just one more minor comment(Sorry!). I'm just thinking to follow Greg's this PR: #124080, probably it's better to rename the MyApp class to AdaptiveAlertDialogApp or something else:)

QuncCccccc

LGTM:)

werainkhatri

LGTM :D

flutter/flutter@42fb0b2...3476b96 2023-04-19 42216813+eliasyishak@users.noreply.github.com Update helper message for `--suppress-analytics` (flutter/flutter#124810) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8b7cdb02f7f3 to 609f9d536494 (1 revision) (flutter/flutter#125097) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 099ed6c62d04 to 8b7cdb02f7f3 (6 revisions) (flutter/flutter#125094) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5fcc7b719029 to 099ed6c62d04 (3 revisions) (flutter/flutter#125078) 2023-04-19 jmccandless@google.com Disableable ContextMenuButtonItems (flutter/flutter#124253) 2023-04-18 58190796+MitchellGoodwin@users.noreply.github.com Adaptive alert dialog (flutter/flutter#124336) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6d263ea56a62 to 5fcc7b719029 (4 revisions) (flutter/flutter#125069) 2023-04-18 58529443+srujzs@users.noreply.github.com Remove package:js/dart:js_interop conflicts (flutter/flutter#124879) 2023-04-18 abadasamuelosp@gmail.com Remove double.fromEnvironment from dart-define doc (flutter/flutter#124102) 2023-04-18 40026920+KKimj@users.noreply.github.com Update to add Kim Jiun to `AUTHORS` (flutter/flutter#125026) 2023-04-18 gspencergoog@users.noreply.github.com Add controller argument to SubmenuButton (flutter/flutter#125000) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 879308a52228 to 6d263ea56a62 (1 revision) (flutter/flutter#125060) 2023-04-18 jmccandless@google.com Limit the number of Material spell check suggestions to 3 (flutter/flutter#124899) 2023-04-18 magder@google.com Remove impeller testowners (flutter/flutter#125056) 2023-04-18 110993981+htoor3@users.noreply.github.com [web] - Clean up skipped tests (flutter/flutter#124981) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 72b68622fffa to 879308a52228 (1 revision) (flutter/flutter#125057) 2023-04-18 goderbauer@google.com Remove unused getRootRenderObject and getSelectedRenderObject service extensions (flutter/flutter#124805) 2023-04-18 thkim1011@users.noreply.github.com l10n.yaml's nullable-getter option should default to true (flutter/flutter#124353) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 55bb065c607b to 72b68622fffa (1 revision) (flutter/flutter#125053) 2023-04-18 47866232+chunhtai@users.noreply.github.com Add vmservice for android build options (flutter/flutter#123034) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com,ychris@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…r#3760) flutter/flutter@42fb0b2...3476b96 2023-04-19 42216813+eliasyishak@users.noreply.github.com Update helper message for `--suppress-analytics` (flutter/flutter#124810) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8b7cdb02f7f3 to 609f9d536494 (1 revision) (flutter/flutter#125097) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 099ed6c62d04 to 8b7cdb02f7f3 (6 revisions) (flutter/flutter#125094) 2023-04-19 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5fcc7b719029 to 099ed6c62d04 (3 revisions) (flutter/flutter#125078) 2023-04-19 jmccandless@google.com Disableable ContextMenuButtonItems (flutter/flutter#124253) 2023-04-18 58190796+MitchellGoodwin@users.noreply.github.com Adaptive alert dialog (flutter/flutter#124336) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 6d263ea56a62 to 5fcc7b719029 (4 revisions) (flutter/flutter#125069) 2023-04-18 58529443+srujzs@users.noreply.github.com Remove package:js/dart:js_interop conflicts (flutter/flutter#124879) 2023-04-18 abadasamuelosp@gmail.com Remove double.fromEnvironment from dart-define doc (flutter/flutter#124102) 2023-04-18 40026920+KKimj@users.noreply.github.com Update to add Kim Jiun to `AUTHORS` (flutter/flutter#125026) 2023-04-18 gspencergoog@users.noreply.github.com Add controller argument to SubmenuButton (flutter/flutter#125000) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 879308a52228 to 6d263ea56a62 (1 revision) (flutter/flutter#125060) 2023-04-18 jmccandless@google.com Limit the number of Material spell check suggestions to 3 (flutter/flutter#124899) 2023-04-18 magder@google.com Remove impeller testowners (flutter/flutter#125056) 2023-04-18 110993981+htoor3@users.noreply.github.com [web] - Clean up skipped tests (flutter/flutter#124981) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 72b68622fffa to 879308a52228 (1 revision) (flutter/flutter#125057) 2023-04-18 goderbauer@google.com Remove unused getRootRenderObject and getSelectedRenderObject service extensions (flutter/flutter#124805) 2023-04-18 thkim1011@users.noreply.github.com l10n.yaml's nullable-getter option should default to true (flutter/flutter#124353) 2023-04-18 engine-flutter-autoroll@skia.org Roll Flutter Engine from 55bb065c607b to 72b68622fffa (1 revision) (flutter/flutter#125053) 2023-04-18 47866232+chunhtai@users.noreply.github.com Add vmservice for android build options (flutter/flutter#123034) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC rmistry@google.com,stuartmorgan@google.com,ychris@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

MitchellGoodwin marked this pull request as ready for review April 6, 2023 19:32

MitchellGoodwin commented Apr 6, 2023

View reviewed changes

MitchellGoodwin requested a review from QuncCccccc April 6, 2023 19:34

QuncCccccc approved these changes Apr 7, 2023

View reviewed changes

MitchellGoodwin force-pushed the adaptive-alert-dialog branch 2 times, most recently from a41e9fe to 52ae952 Compare April 7, 2023 17:27

werainkhatri reviewed Apr 8, 2023

View reviewed changes

MitchellGoodwin force-pushed the adaptive-alert-dialog branch from 52ae952 to 2aa7c6d Compare April 10, 2023 18:10

werainkhatri reviewed Apr 10, 2023

View reviewed changes

QuncCccccc reviewed Apr 18, 2023

View reviewed changes

MitchellGoodwin force-pushed the adaptive-alert-dialog branch from c0b1485 to d875fcd Compare April 18, 2023 19:04

QuncCccccc reviewed Apr 18, 2023

View reviewed changes

MitchellGoodwin added 11 commits April 18, 2023 13:18

Start of adaptive dialog

851a04a

Add documentation

85c1836

Add tests

05031b5

Add example and tests

bf03e6f

Small fixes

f30ac5c

Add cupertino specific properties to adaptive constructor

0f98870

Remove unused variable

253f8b3

Adjust documentation

e212ee4

Adjust documentation

df7a013

Refactor to use factory

7bd1a37

Change example app name

acececd

MitchellGoodwin force-pushed the adaptive-alert-dialog branch from d875fcd to acececd Compare April 18, 2023 20:18

QuncCccccc approved these changes Apr 18, 2023

View reviewed changes

werainkhatri approved these changes Apr 18, 2023

View reviewed changes

MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 18, 2023

auto-submit bot merged commit bd2617e into flutter:master Apr 18, 2023
68 checks passed

engine-flutter-autoroll mentioned this pull request Apr 19, 2023

Roll Flutter from 42fb0b23139c to 3476b966524f (20 revisions) flutter/packages#3760

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 19, 2023

bd2617ecb Adaptive alert dialog (flutter/flutter#124336)

cf47d2d

This was referenced Apr 20, 2023

i123636 android embedding v2 agp gradle kotlin update #125256

Closed

[integration_test] add support for agp8.0 #125310

Closed

HansMuller mentioned this pull request Apr 21, 2023

Flutter Framework Team: Notable Commits #121415

Open

reidbaker mentioned this pull request Apr 21, 2023

Test AGP 8.0 using java 17 #125323

Merged

8 tasks

JvnSlv mentioned this pull request Aug 24, 2023

Fix: Integration test adjustments Prime-Holding/rx_bloc#567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adaptive alert dialog #124336

Adaptive alert dialog #124336

		@@ -24,6 +24,8 @@ import 'theme_data.dart';

		const EdgeInsets _defaultInsetPadding = EdgeInsets.symmetric(horizontal: 40.0, vertical: 24.0);

		enum _DialogType {material, adaptive }

	enum _DialogType {material, adaptive }
	enum _DialogType { material, adaptive }

Adaptive alert dialog #124336

Adaptive alert dialog #124336

Conversation

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment