-
Notifications
You must be signed in to change notification settings - Fork 350
dialog: Use Cupertino-flavored alert dialogs on iOS #1017
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
Conversation
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
Also, please tidy up the branch's commit history for clear and coherent commits. Each commit should be clean and pass all tests (you can run our tests with tools/check).
lib/widgets/dialog.dart
Outdated
| /// Sets the dialog action to be platform appropriate | ||
| /// by displaying a [CupertinoDialogAction] for IOS platforms | ||
| /// and a regular [TextButton] otherwise. | ||
| Widget _adaptiveAction( | ||
| {required BuildContext context, | ||
| required VoidCallback onPressed, | ||
| required Widget child}) { | ||
| final ThemeData theme = Theme.of(context); | ||
| switch (theme.platform) { | ||
| case TargetPlatform.android: | ||
| return TextButton(onPressed: onPressed, child: child); | ||
| case TargetPlatform.fuchsia: | ||
| return TextButton(onPressed: onPressed, child: child); | ||
| case TargetPlatform.linux: | ||
| return TextButton(onPressed: onPressed, child: child); | ||
| case TargetPlatform.windows: | ||
| return TextButton(onPressed: onPressed, child: child); | ||
| case TargetPlatform.iOS: | ||
| return CupertinoDialogAction(onPressed: onPressed, child: child); | ||
| case TargetPlatform.macOS: | ||
| return CupertinoDialogAction(onPressed: onPressed, child: child); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be tightened up in a few ways:
-
Use empty
cases to fall through. From the doc on Dartswitchstatements: -
Use
defaultTargetPlatforminstead of passing throughcontext. When the app is run on a device,defaultTargetPlatformwill match the platform (iOS or Android). When we need to simulate a specific platform in tests, we setdefaultTargetPlatform; search fordebugDefaultTargetPlatformOverridefor how we do this. -
More concise dartdoc
Also, because the logic in _dialogActionText fits and is recommended for the Material-style dialog, let's only apply it on Android. This helper is a fine place for that conditional logic; how about changing its interface so it takes a String instead of a Widget, and applies _dialogActionText in the Android branch. In my proposal below, I've also given _dialogActionText an appropriately specific name, _materialDialogActionText. That helper could even be inlined, perhaps in a followup NFC commit.
So, putting all that together:
| /// Sets the dialog action to be platform appropriate | |
| /// by displaying a [CupertinoDialogAction] for IOS platforms | |
| /// and a regular [TextButton] otherwise. | |
| Widget _adaptiveAction( | |
| {required BuildContext context, | |
| required VoidCallback onPressed, | |
| required Widget child}) { | |
| final ThemeData theme = Theme.of(context); | |
| switch (theme.platform) { | |
| case TargetPlatform.android: | |
| return TextButton(onPressed: onPressed, child: child); | |
| case TargetPlatform.fuchsia: | |
| return TextButton(onPressed: onPressed, child: child); | |
| case TargetPlatform.linux: | |
| return TextButton(onPressed: onPressed, child: child); | |
| case TargetPlatform.windows: | |
| return TextButton(onPressed: onPressed, child: child); | |
| case TargetPlatform.iOS: | |
| return CupertinoDialogAction(onPressed: onPressed, child: child); | |
| case TargetPlatform.macOS: | |
| return CupertinoDialogAction(onPressed: onPressed, child: child); | |
| } | |
| } | |
| /// A platform-appropriate action for [AlertDialog.adaptive]'s [actions] param. | |
| Widget _adaptiveAction({required VoidCallback onPressed, required String text}) { | |
| switch (defaultTargetPlatform) { | |
| case TargetPlatform.android: | |
| case TargetPlatform.fuchsia: | |
| case TargetPlatform.linux: | |
| case TargetPlatform.windows: | |
| return TextButton(onPressed: onPressed, child: _materialDialogActionText(text)); | |
| case TargetPlatform.iOS: | |
| case TargetPlatform.macOS: | |
| return CupertinoDialogAction(onPressed: onPressed, child: Text(text)); | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the idea for inlining the _materialDialogActionText?
return TextButton(onPressed: onPressed, child: Text(text, textAlign: TextAlign.end));will update the commit message of the latest commit as well as per the guidelines
test/widgets/dialog_checks.dart
Outdated
| /// | ||
| /// On success, returns the widget's "OK" button. | ||
| /// On success, returns the widget's "OK" button | ||
| /// (which is a [CupertinoDialogAction] for OS platforms). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave this dartdoc unchanged. It doesn't matter what specific widget the button is, as long as it responds to taps.
| final dialog = tester.widget<AlertDialog>(find.byType(AlertDialog)); | ||
| tester.widget(find.descendant(matchRoot: true, | ||
| of: find.byWidget(dialog.title!), matching: find.text(expectedTitle))); | ||
| if (expectedMessage != null) { | ||
| if (defaultTargetPlatform == TargetPlatform.iOS | ||
| || defaultTargetPlatform == TargetPlatform.macOS) { | ||
|
|
||
| final dialog = tester.widget<CupertinoAlertDialog>(find.byType(CupertinoAlertDialog)); | ||
|
|
||
| tester.widget(find.descendant(matchRoot: true, | ||
| of: find.byWidget(dialog.content!), matching: find.text(expectedMessage))); | ||
| } | ||
| of: find.byWidget(dialog.title!), matching: find.text(expectedTitle))); | ||
|
|
||
| return tester.widget( | ||
| find.descendant(of: find.byWidget(dialog), | ||
| matching: find.widgetWithText(TextButton, 'OK'))); | ||
| if (expectedMessage != null) { | ||
| tester.widget(find.descendant(matchRoot: true, | ||
| of: find.byWidget(dialog.content!), matching: find.text(expectedMessage))); | ||
| } | ||
|
|
||
| return tester.widget( | ||
| find.descendant(of: find.byWidget(dialog), | ||
| matching: find.widgetWithText(CupertinoDialogAction, 'OK'))); | ||
|
|
||
| } | ||
| else { | ||
| final dialog = tester.widget<Dialog>(find.byType(Dialog)); | ||
| tester.widget(find.widgetWithText(Dialog, expectedTitle)); | ||
| if (expectedMessage != null) { | ||
| tester.widget(find.widgetWithText(Dialog, expectedMessage)); | ||
| } | ||
| return tester.widget( | ||
| find.descendant(of: find.byWidget(dialog), | ||
| matching: find.widgetWithText(TextButton, 'OK'))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use an exhaustive switch on defaultTargetPlatform, like we do elsewhere. Also, there are several formatting nits that makes this code harder to read than it needs to be.
Proposal:
switch (defaultTargetPlatform) {
case TargetPlatform.android:
case TargetPlatform.fuchsia:
case TargetPlatform.linux:
case TargetPlatform.windows: {
final dialog = tester.widget<Dialog>(find.byType(Dialog));
tester.widget(find.widgetWithText(Dialog, expectedTitle));
if (expectedMessage != null) {
tester.widget(find.widgetWithText(Dialog, expectedMessage));
}
return tester.widget(
find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(TextButton, 'OK')));
}
case TargetPlatform.iOS:
case TargetPlatform.macOS: {
final dialog = tester.widget<CupertinoAlertDialog>(
find.byType(CupertinoAlertDialog));
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.title!), matching: find.text(expectedTitle)));
if (expectedMessage != null) {
tester.widget(find.descendant(matchRoot: true,
of: find.byWidget(dialog.content!), matching: find.text(expectedMessage)));
}
return tester.widget(find.descendant(of: find.byWidget(dialog),
matching: find.widgetWithText(CupertinoDialogAction, 'OK')));
}
}| description: | ||
| name: file | ||
| sha256: "5fc22d7c25582e38ad9a8515372cd9a93834027aacf1801cf01164dac0ffa08c" | ||
| sha256: a3b4f84adafef897088c160faf7dfffb7696046cb13ae90b508c2cbc95d3b8d4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file don't look related; please remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thankyou for the feedback, will be sure to onboard these points when working on issues in the future. I'll fix these things up in a new commit.
|
(Misclick, sorry) |
a255919 to
db740f9
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. You'll need to tidy up your branch, as I mentioned above, before we can review this again. If you need help, please ask in #git help in the development community.
yep my bad, will do so. Thanks for your patients with me on this. |
ed8abb7 to
9d2c4df
Compare
As was suggested in a comment of the pull request zulip#1017 (comment).
9e6fc98 to
373cd75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is much closer! Small comments below.
Also, a few commit message nits:
dialog: display adaptive dialogs and action buttons based on the target platform
AlertDialog was changed to AlertDialog.adaptive to the effect described in #996.
_adaptiveAction was implemented to display a platform appropriate action for
AlertDialog.adaptive's actions param, as was also discussed in #996.
tests in dialog_test were updated to perform platform appropriate tests.
- This commit fixes an issue (🎉), so let's put a
Fixes: #996line at the end of it. - Also, I think the paragraph ("AlertDialog was changed…") doesn't add anything that's not already obvious from reading the code changes and the linked issue, so let's just delete it.
dialog [nfc]: inline _materialDialogActionTest in _adaptiveAction
As was suggested in a comment of the pull request https://github.com/zulip/zulip-flutter/pull/1017#discussion_r1813819656.
-
The URL should be on a new line; we try to wrap to 68 columns except where doing so would make things more confusing. How about:
As suggested at: https://github.com/zulip/zulip-flutter/pull/1017#discussion_r1813819656
Then for both commit messages, use initial caps for the part after the prefix, so:
dialog: Display adaptive dialogs and action buttons based on the target platformdialog [nfc]: inline _materialDialogActionTest in _adaptiveAction
For examples of commit messages in the project's style, see the project's Git history; I recommend Greg's excellent tip about how to do that.
As was suggested in a comment of the pull request zulip#1017 (comment).
373cd75 to
52d762d
Compare
52d762d to
6166a0a
Compare
6166a0a to
f91a8c5
Compare
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Some new nits below, and also some more commit-message nits that I should have caught last time 🙂:
dialog: Display adaptive dialogs and action buttons based on the target platform
This summary line is too long, at 80 characters. How about:
dialog: Use Cupertino-flavored alert dialogs on iOS
This also has a bit more information: the fact that iOS was the platform getting the wrong-style dialog. (The fact that the dialog's buttons match the rest of the dialog isn't surprising enough to need a mention. 🙂)
For what the length limit actually is, see discussion, which I've just started 🙂. This was a helpful opportunity for me to spot a change in the zulip/zulip documentation that I'd missed!
dialog [nfc]: Inline _materialDialogActionTest in _adaptiveAction
The function's name is _materialDialogActionText, not _materialDialogActionTest.
f91a8c5 to
0a44828
Compare
all sorted :) |
|
@BrynMtchll I'm guessing you're the same person as @u7088495? Please pick a single account to stick to, at least for interacting with any given project (like Zulip) — it makes things less confusing 🙂 It looks like this has some merge/rebase conflicts after 0094978 / #1410. Would you rebase and resolve those? Then I think this will be all ready for merge. |
|
Sorry yes that's my other account, my mistake - I forgot that I switched over for a different project |
991a4fe to
5e56125
Compare
5e56125 to
4af9eb5
Compare
4af9eb5 to
39a6ffe
Compare
|
Alright I think it's ready; I refactored the learn more test logic into |
gnprice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! There's one substantive change needed in the new test code, and a couple of easy nits.
lib/widgets/dialog.dart
Outdated
| text: zulipLocalizations.errorDialogLearnMore, | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: preserve formatting
| text: zulipLocalizations.errorDialogLearnMore, | |
| ), | |
| text: zulipLocalizations.errorDialogLearnMore), |
| textAlign: TextAlign.end)); | ||
| case TargetPlatform.iOS: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: a blank line to separate these cases helps make the structure easier to see:
| textAlign: TextAlign.end)); | |
| case TargetPlatform.iOS: | |
| textAlign: TextAlign.end)); | |
| case TargetPlatform.iOS: |
test/widgets/dialog_checks.dart
Outdated
| if (expectedLearnMoreButtonUrl != null) { | ||
| check(testBinding.takeLaunchUrlCalls()).single.equals(( | ||
| url: expectedLearnMoreButtonUrl, | ||
| mode: LaunchMode.inAppBrowserView)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for this button is useful, but this check doesn't work in this context. The check will work only if the caller has already gone and tapped on the button. There's nothing in this function's dartdoc that says the caller should do that, and it's not something a reader would naturally assume they need to do.
If a caller is going and tapping on the button themself, then the caller can also easily go and do this check — the check isn't using any specific knowledge about how the error dialogs work.
It looks like there's one call site that passes this option. So let's move this check to that call site.
39a6ffe to
a149fef
Compare
a149fef to
1a77779
Compare
|
hey @gnprice, sorry I've stepped away from this - I've come back to it and think it's ready for another review now :). |
gnprice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the revision! Small comments.
test/widgets/dialog_test.dart
Outdated
| switch (defaultTargetPlatform) { | ||
| case TargetPlatform.android: | ||
| case TargetPlatform.fuchsia: | ||
| case TargetPlatform.linux: | ||
| case TargetPlatform.windows: | ||
| check(testBinding.takeLaunchUrlCalls()).single.equals(( | ||
| url: learnMoreButtonUrl, | ||
| mode: LaunchMode.inAppBrowserView)); | ||
| case TargetPlatform.iOS: | ||
| case TargetPlatform.macOS: | ||
| check(testBinding.takeLaunchUrlCalls()).single.equals(( | ||
| url: learnMoreButtonUrl, | ||
| mode: LaunchMode.externalApplication)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fairly verbose, in a way that obscures the fact that the only difference between the cases is the value of mode.
Instead, let's first determine teh mode we expect; then have just one, more focused, check that calls takeLaunchUrlCalls.
For examples, search the existing tests in test/ for "LaunchMode.externalApplication".
test/widgets/dialog_test.dart
Outdated
| await tester.tap(find.text('Learn more')); | ||
| check(testBinding.takeLaunchUrlCalls()).single.equals(( | ||
| url: Uri.parse('https://foo.example'), | ||
| mode: LaunchMode.inAppBrowserView)); | ||
| }); | ||
|
|
||
| checkErrorDialog(tester, expectedTitle: title); | ||
|
|
||
| switch (defaultTargetPlatform) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These steps are out of their logical order. The dialog must be there before the tester.tap on one of its buttons can work; so let's check for the dialog before trying to tap one of its buttons.
1a77779 to
f369cc7
Compare
|
alright should be ready for another revision :) |
gnprice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @u7088495 for all your work on this! All looks good now except a couple of nits below. I'll fix those and merge.
| .equals((url: learnMoreButtonUrl, mode: expectedMode)); | ||
|
|
||
| }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no blank line at end of block
| .equals((url: learnMoreButtonUrl, mode: expectedMode)); | |
| }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); | |
| .equals((url: learnMoreButtonUrl, mode: expectedMode)); | |
| }, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS})); |
| checkErrorDialog(tester, expectedTitle: title); | ||
| await tester.tap(find.text('Learn more')); | ||
| check(testBinding.takeLaunchUrlCalls()).single.equals(( | ||
| url: Uri.parse('https://foo.example'), | ||
| mode: LaunchMode.inAppBrowserView)); | ||
| }); | ||
|
|
||
| final expectedMode = switch (defaultTargetPlatform) { | ||
| TargetPlatform.android => LaunchMode.inAppBrowserView, | ||
| TargetPlatform.iOS => LaunchMode.externalApplication, | ||
| _ => throw StateError('attempted to test with $defaultTargetPlatform'), | ||
| }; | ||
|
|
||
| check(testBinding.takeLaunchUrlCalls()).single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use blank lines to group test steps into stanzas of set up, then check.
See #1317 (comment) and the previous comments linked from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the adjusted version of this test which I just merged:
testWidgets('tap "Learn more" button', (tester) async {
await prepare(tester);
final learnMoreButtonUrl = Uri.parse('https://foo.example');
showErrorDialog(context: context, title: title, learnMoreButtonUrl: learnMoreButtonUrl);
await tester.pump();
checkErrorDialog(tester, expectedTitle: title);
await tester.tap(find.text('Learn more'));
final expectedMode = switch (defaultTargetPlatform) {
TargetPlatform.android => LaunchMode.inAppBrowserView,
TargetPlatform.iOS => LaunchMode.externalApplication,
_ => throw StateError('attempted to test with $defaultTargetPlatform'),
};
check(testBinding.takeLaunchUrlCalls()).single
.equals((url: learnMoreButtonUrl, mode: expectedMode));
}, variant: const TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));Note how each stanza (each group separated by blank lines) first sets up a situation, then checks that it's as expected.
f369cc7 to
ec1baee
Compare
In zulip#1017, we overlooked the fact that a SingleChildScrollView is added automatically on iOS but not on Android. I haven't reproduced an observable bug that comes from this, but it calls for a fix. I tested this change manually on iOS (showErrorDialog, showSuggestedActionDialog, and UpgradeWelcomeDialog), with short text and long text (longer than a screenful, to check the scrolling works).
In zulip#1017, we overlooked the fact that a SingleChildScrollView is added automatically on iOS but not on Android. I haven't reproduced an observable bug that comes from this, but it calls for a fix. I tested this change manually on iOS (showErrorDialog, showSuggestedActionDialog, and UpgradeWelcomeDialog), with short text and long text (longer than a screenful, to check the scrolling works).
In zulip#1017, we overlooked the fact that a SingleChildScrollView is added automatically on iOS but not on Android. I haven't reproduced an observable bug that comes from this, but it calls for a fix. I tested this change manually on iOS (showErrorDialog, showSuggestedActionDialog, and UpgradeWelcomeDialog), with short text and long text (longer than a screenful, to check the scrolling works).
This pull request closes #996.
In
dialog.dart:Switched
alertDialog(toalertDialog.adaptive(, as per #996.Defined new private widget
_adaptiveActionwhich displays aCupertinoDialogActionfor IOS and aTextButtonotherwise._adaptiveAction(is used in place ofTextButton(when stating dialog actions.The new result for IOS:

dialog-checks.darthas been updated to check the text content of the respective dialog types depending on the platform