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

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

Open
chrisbobbe opened this issue Oct 11, 2024 · 6 comments · May be fixed by #1017
Open

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

chrisbobbe opened this issue Oct 11, 2024 · 6 comments · May be fixed by #1017
Labels
a-design Visual and UX design a-iOS Issues specific to iOS, or requiring iOS-specific work
Milestone

Comments

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Oct 11, 2024

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.

@chrisbobbe chrisbobbe added a-iOS Issues specific to iOS, or requiring iOS-specific work a-design Visual and UX design labels Oct 11, 2024
@chrisbobbe chrisbobbe added this to the Post-launch milestone Oct 11, 2024
@u7088495
Copy link

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
Copy link
Member

gnprice commented Oct 19, 2024

@u7088495
Copy link

u7088495 commented Oct 21, 2024

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
Copy link
Collaborator Author

chrisbobbe commented Oct 21, 2024

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
Copy link

u7088495 commented Oct 22, 2024

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?

@u7088495 u7088495 linked a pull request Oct 22, 2024 that will close this issue
@gnprice
Copy link
Member

gnprice commented Oct 23, 2024

@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.

u7088495 added a commit to u7088495/zulip-flutter that referenced this issue Oct 25, 2024
…et platform

AlertDialog was changed to AlertDialog.adaptive to the effect described in zulip#996.
_adaptiveAction was implemented to display a platform appropriate action for
AlertDialog.adaptive's actions param, as was also discussed in zulip#996.
tests in dialog_test were updated to perform platform appropriate tests.
u7088495 added a commit to u7088495/zulip-flutter that referenced this issue Oct 26, 2024
u7088495 added a commit to u7088495/zulip-flutter that referenced this issue Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-design Visual and UX design a-iOS Issues specific to iOS, or requiring iOS-specific work
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

3 participants