zulip / zulip-flutter

Upcoming Zulip mobile apps for Android and iOS, using Flutter
Apache License 2.0
177 stars 164 forks source link

dialog: Use Cupertino-flavored alert dialogs on iOS #996

Open chrisbobbe opened 2 weeks ago

chrisbobbe commented 2 weeks ago

We should experiment with the AlertDialog.adaptive constructor, instead of AlertDialog, which forces a Material-style dialog on iOS:

Current Should be more like
image image

The "after" screenshot was made by simply changing AlertDialog( to AlertDialog.adaptive(. A complete fix will be a little more complicated than that, though. See the code sample on the AlertDialog.adaptive doc, which shows a platform switch for the dialog's action buttons, to style those appropriately (including the buttons' on-press appearance):

  Widget adaptiveAction(
      {required BuildContext context,
      required VoidCallback onPressed,
      required Widget child}) {
    final ThemeData theme = Theme.of(context);
    switch (theme.platform) {
      case TargetPlatform.android:
      case TargetPlatform.fuchsia:
      case TargetPlatform.linux:
      case TargetPlatform.windows:
        return TextButton(onPressed: onPressed, child: child);
      case TargetPlatform.iOS:
      case TargetPlatform.macOS:
        return CupertinoDialogAction(onPressed: onPressed, child: child);
    }
  }

Relatedly, I think the TextAlign.end in our _dialogActionText helper should not be applied on iOS because on iOS the button text is meant to be center-aligned.

I also notice that our "Source Sans 3" font isn't being applied in the Cupertino-style dialog in the screenshot. I think we want it to be, so we should debug and fix this. Probably should leave this be, actually; it's supposed to mimic the native iOS alert, which uses a system font, not an app's chosen font.

u7088495 commented 1 week ago

Hi I'm keen to get involved with this project and this issue looks like a good place to start, can I have a go at it?

gnprice commented 1 week ago

@u7088495 Please take a look at the Zulip project's guide to getting involved with the code: https://zulip.readthedocs.io/en/latest/contributing/contributing.html#your-first-codebase-contribution https://zulip.readthedocs.io/en/latest/contributing/contributing.html#picking-an-issue-to-work-on

u7088495 commented 4 days ago

As demonstrated, in dialog.dart I'd replace AlertDialog( with AlertDialog.adaptive(. I'd also implement the widget adaptiveAction to return a CupertinoDialogAction( for OS platforms and the regular textButton otherwise. As for the font, I am unsure how to fix that rather than by setting the textStyle property on the Text widgets directly - I tried defining cupertino theme data but with no luck.

chrisbobbe commented 4 days ago

Sounds good!

As for the font, I am unsure how to fix that rather than by setting the textStyle property on the Text widgets directly - I tried defining cupertino theme data but with no luck.

I think it may be fine, actually, to accept the default styling, instead of overriding it with our custom styles with "Source Sans 3".

If we were using a native iOS API to show these alerts, we'd get whatever system-defined styles Apple chooses—like what's illustrated in this interface guidelines doc from Apple—and Flutter's defaults understandably mimic those.

u7088495 commented 4 days ago

Also, on the text alignment, with the CupertinoDialogAction, the label doesn't wrap and so setting TextAlign.end has no effect.

Screenshot 2024-10-22 at 1 59 04 pm

But I could still update this if its worth being explicit?

gnprice commented 2 days ago

@u7088495 I think the most effective venue for getting that question answered would be in chat, in #mobile-dev-help. That will also help keep this issue thread focused.