Adaptive alert dialog by MitchellGoodwin · Pull Request #124336 · flutter/flutter

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

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

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

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

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

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

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

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

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

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

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 [email protected] Update helper message for `--suppress-analytics` (flutter/flutter#124810) 2023-04-19 [email protected] Roll Flutter Engine from 8b7cdb02f7f3 to 609f9d536494 (1 revision) (flutter/flutter#125097) 2023-04-19 [email protected] Roll Flutter Engine from 099ed6c62d04 to 8b7cdb02f7f3 (6 revisions) (flutter/flutter#125094) 2023-04-19 [email protected] Roll Flutter Engine from 5fcc7b719029 to 099ed6c62d04 (3 revisions) (flutter/flutter#125078) 2023-04-19 [email protected] Disableable ContextMenuButtonItems (flutter/flutter#124253) 2023-04-18 [email protected] Adaptive alert dialog (flutter/flutter#124336) 2023-04-18 [email protected] Roll Flutter Engine from 6d263ea56a62 to 5fcc7b719029 (4 revisions) (flutter/flutter#125069) 2023-04-18 [email protected] Remove package:js/dart:js_interop conflicts (flutter/flutter#124879) 2023-04-18 [email protected] Remove double.fromEnvironment from dart-define doc (flutter/flutter#124102) 2023-04-18 [email protected] Update to add Kim Jiun to `AUTHORS` (flutter/flutter#125026) 2023-04-18 [email protected] Add controller argument to SubmenuButton (flutter/flutter#125000) 2023-04-18 [email protected] Roll Flutter Engine from 879308a52228 to 6d263ea56a62 (1 revision) (flutter/flutter#125060) 2023-04-18 [email protected] Limit the number of Material spell check suggestions to 3 (flutter/flutter#124899) 2023-04-18 [email protected] Remove impeller testowners (flutter/flutter#125056) 2023-04-18 [email protected] [web] - Clean up skipped tests (flutter/flutter#124981) 2023-04-18 [email protected] Roll Flutter Engine from 72b68622fffa to 879308a52228 (1 revision) (flutter/flutter#125057) 2023-04-18 [email protected] Remove unused getRootRenderObject and getSelectedRenderObject service extensions (flutter/flutter#124805) 2023-04-18 [email protected] l10n.yaml's nullable-getter option should default to true (flutter/flutter#124353) 2023-04-18 [email protected] Roll Flutter Engine from 55bb065c607b to 72b68622fffa (1 revision) (flutter/flutter#125053) 2023-04-18 [email protected] 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 [email protected],[email protected],[email protected] 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

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

bd2617e 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

Closed

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

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

Conversation

MitchellGoodwin commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

werainkhatri Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

werainkhatri Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

werainkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MitchellGoodwin commented Apr 6, 2023 •

edited

Loading

werainkhatri Apr 10, 2023 •

edited

Loading

werainkhatri Apr 10, 2023 •

edited

Loading