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

Add support for full screen dialogs #104961

Closed
Tracked by #91605
guidezpl opened this issue May 30, 2022 · 16 comments · Fixed by #112261
Closed
Tracked by #91605

Add support for full screen dialogs #104961

guidezpl opened this issue May 30, 2022 · 16 comments · Fixed by #112261
Assignees
Labels
c: new feature Nothing broken; request for a new capability f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects

Comments

@guidezpl
Copy link
Member

Spec

image

@guidezpl guidezpl added c: new feature Nothing broken; request for a new capability framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels May 30, 2022
@HansMuller
Copy link
Contributor

Part of #91605

@TahaTesser
Copy link
Member

This is required for #106101, looking at what is needed to make this feature possible.

@TahaTesser TahaTesser added this to To do in Nevercode via automation Jun 17, 2022
@TahaTesser TahaTesser self-assigned this Jun 17, 2022
@TahaTesser
Copy link
Member

cc: @guidezpl @darrenaustin

Do you think creating a new FullscreenDialog widget is the way to go to make it work out of the box? or perhaps adding something like Dialog.fullscreen().

We can create a fullscreen dialog using Dialog widget with some zero insetPadding and a Scaffold. If this is sufficient, we can close this issue.

Code Sample
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key, this.dark = true, this.useMaterial3 = true});

  final bool dark;
  final bool useMaterial3;

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      themeMode: dark ? ThemeMode.dark : ThemeMode.light,
      theme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.light,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      darkTheme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.dark,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      home: const DialogExample(),
    );
  }
}

class DialogExample extends StatelessWidget {
  const DialogExample({super.key});

  Future<void> _dialog(BuildContext context) async {
    return showDialog(
      context: context,
      builder: (BuildContext context) {
        return Dialog(
          insetPadding: EdgeInsets.zero,
          child: Scaffold(
            appBar: AppBar(
              title: const Text('Full-screen dialog title'),
              centerTitle: false,
              automaticallyImplyLeading: false,
              leading: IconButton(
                onPressed: () => Navigator.of(context).pop(),
                icon: const Icon(Icons.close),
              ),
              actions: <Widget>[
                TextButton(
                  onPressed: () => Navigator.of(context).pop(),
                  child: const Text('Save'),
                ),
              ],
            ),
          ),
        );
      },
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Dialog'),
      ),
      body: Center(
        child: ElevatedButton(
          onPressed: () => _dialog(context),
          child: const Text('Open Full-screen Dialog'),
        ),
      ),
    );
  }
}
Screen.Recording.2022-07-13.at.13.49.40.mov

@guidezpl
Copy link
Member Author

It does look very close, but not quite up to spec (save button padding). I do like Dialog.fullscreen() for this kind of case.

Can you add some content to the example? I'd like to see how close to spec we are by default.

@TahaTesser
Copy link
Member

TahaTesser commented Jul 14, 2022

It's getting there but you're right, it's not quite up to spec, given the fullscreen_dialog specs and tokens. Now I think matching the specs is the way perhaps with Dialog.fullscreen()

Code Sample
import 'package:flutter/material.dart';

import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key, this.dark = true, this.useMaterial3 = true});

  final bool dark;
  final bool useMaterial3;

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      debugShowCheckedModeBanner: false,
      themeMode: dark ? ThemeMode.dark : ThemeMode.light,
      theme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.light,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      darkTheme: ThemeData(
        useMaterial3: useMaterial3,
        brightness: Brightness.dark,
        colorSchemeSeed: const Color(0xff6750a4),
      ),
      home: const Example(),
    );
  }
}

class Example extends StatefulWidget {
  const Example({super.key});

  @override
  State<Example> createState() => _ExampleState();
}

class _ExampleState extends State<Example> {
  late TextEditingController _controller;

  @override
  void initState() {
    super.initState();
    _controller = TextEditingController(text: 'Flutter is amazing!');
  }

  @override
  void dispose() {
    _controller.dispose();
    super.dispose();
  }

  Future<void> _dialog(BuildContext context) async {
    return showDialog(
      context: context,
      builder: (BuildContext context) {
        return Dialog(
          insetPadding: EdgeInsets.zero,
          child: Scaffold(
            appBar: AppBar(
              title: const Text('Full-screen dialog title'),
              centerTitle: false,
              automaticallyImplyLeading: false,
              leading: IconButton(
                onPressed: () => Navigator.of(context).pop(),
                icon: const Icon(Icons.close),
              ),
              actions: <Widget>[
                TextButton(
                  onPressed: () => Navigator.of(context).pop(),
                  child: const Text('Save'),
                ),
              ],
            ),
            body: Padding(
              padding: const EdgeInsets.all(16.0),
              child: Column(
                children: <Widget>[
                  TextField(
                    controller: _controller,
                    decoration: const InputDecoration(
                        border: OutlineInputBorder(),
                        suffixIcon: Icon(Icons.arrow_drop_down)),
                  ),
                  const SizedBox(height: 20),
                  TextField(
                    controller: _controller,
                    decoration: const InputDecoration(
                      border: OutlineInputBorder(),
                    ),
                  ),
                ],
              ),
            ),
          ),
        );
      },
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: const Text('Dialog'),
      ),
      body: Center(
        child: ElevatedButton(
          onPressed: () => _dialog(context),
          child: const Text('Open Full-screen Dialog'),
        ),
      ),
    );
  }
}
Preview

@TahaTesser
Copy link
Member

TahaTesser commented Jul 14, 2022

Dialog is actually an unopinionated dialog.

In the Flutter context, AlertDialog is the dialog that provides the ability title, message, and actions buttons and the M3 Fullscreen dialog is similar to that but of course fullscreen.

Also, AlertDialog has quite many parameters that are not related to Fullscreen Dialog

In this case, creating a new FullscreenDialog makes more sense to me (Dialog being unopinionated means it has none of the features to support M3 tokens)

Previously I migrated AlertDialog to M3 with tokens.

Screenshot 2022-07-14 at 16 11 28

@guidezpl
Copy link
Member Author

guidezpl commented Jul 14, 2022

Agreed that AlertDialog isn't the way to go for this. Since Dialog

In this case, creating a new FullscreenDialog makes more sense to me (Dialog being unopinionated means it has none of the features to support M3 tokens)

I'm not sure I understand this. Since Dialog is so close, wouldn't it make more sense to migrate it? We could have Dialog.fullscreen which sets insetPadding and a new padding or contentPadding to match spec, leaving the rest up to users.

Also, if you overlay the sample with the spec, is the padding around the button the only padding difference?

@TahaTesser
Copy link
Member

TahaTesser commented Jul 15, 2022

I'm not sure I understand this. Since Dialog is so close, wouldn't it make more sense to migrate it? We could have Dialog.fullscreen which sets insetPadding and a new padding or contentPadding to match spec, leaving the rest up to users.

That's what I thought at first, given Android doesn't provide a concrete fullscreen implementation.

According to Dialogs - Full-screen dialogs and tokens, there needs to be a header with the title with app bar like height, the closing icon, and action text button and it should also elevate on the scroll and a divider.

AlertDialog follows its specs so should fullscreen dialog given the similar tokens and specs above?

Could we leave all this full-screen dialog design for the users to match the specs?

I think the new FullscreenDialog that matches specs and uses the tokens to style header, icon, action button, and the divider is probably good? or I can create Dialog.fullscreen, and let the user create their header and divider using the AppBar widget?

@guidezpl
Copy link
Member Author

Both options are valid, but I suspect the customizeability of full screen dialogs is more important than providing an exact match to spec. I'd lean towards the Dialog.fullscreen approach.

@TahaTesser
Copy link
Member

TahaTesser commented Jul 15, 2022

@guidezpl

Thanks so much! This helps a lot.

@pedromassango
Copy link
Member

I was about to file a new issue for this :) Is someone working on this up?

@TahaTesser
Copy link
Member

@pedromassango
I have the code ready to be filed. (I was on vacation and working on higher priority issues).

@TahaTesser
Copy link
Member

TahaTesser commented Aug 22, 2022

@pedromassango
I have implemented Dialog.fullscreen based on the discussion. I will file the PR soon and would like it if you could take a look.

@pedromassango
Copy link
Member

That is great 🏅

@TahaTesser
Copy link
Member

Update:
Didn't forget about this. I was focused on getting some existing PRs merged.
Filing this dialog tomorrow.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
Status: Done (PR merged)
Status: Done
Nevercode
  
To do
Development

Successfully merging a pull request may close this issue.

4 participants