Skip to content

Fix Material 3 AppBar.leading action IconButtons - #154512

Merged
auto-submit[bot] merged 2 commits into
flutter:masterfrom
TahaTesser:fix_m3_app_bar_leading_size
Sep 6, 2024
Merged

auto-submit[bot] merged 2 commits into
flutter:masterfrom
TahaTesser:fix_m3_app_bar_leading_size

Conversation

@TahaTesser

@TahaTesser TahaTesser commented Sep 2, 2024 •

Copy link
Copy Markdown
Contributor

Fixes AppBar back button focus/hover circle should not fill up whole height
Fixes [Material 3] Date Range Picker close button has incorrect shape

This updates the leading condition added in #110722

Code sample

expand to view the code sample
import 'package:flutter/material.dart';

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

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

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        body: SingleChildScrollView(
          child: Column(
            children: [
              Column(
                spacing: 10.0,
                mainAxisSize: MainAxisSize.min,
                children: <Widget>[
                  AppBar(
                    leading: BackButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    title: const Text('AppBar with BackButton'),
                  ),
                  AppBar(
                    leading: CloseButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    title: const Text('AppBar with CloseButton'),
                  ),
                  AppBar(
                    leading: DrawerButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    title: const Text('AppBar with DrawerButton'),
                  ),
                ],
              ),
              const Divider(),
              Column(
                spacing: 10.0,
                mainAxisSize: MainAxisSize.min,
                children: <Widget>[
                  AppBar(
                    leading: BackButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    toolbarHeight: 100.0,
                    title: const Text('AppBar with custom height'),
                  ),
                  AppBar(
                    leading: CloseButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    toolbarHeight: 100.0,
                    title: const Text('AppBar with custom height'),
                  ),
                  AppBar(
                    leading: DrawerButton(
                      style: IconButton.styleFrom(backgroundColor: Colors.red),
                    ),
                    backgroundColor: Theme.of(context).colorScheme.secondaryContainer,
                    toolbarHeight: 100.0,
                    title: const Text('AppBar with custom height'),
                  ),
                ],
              ),
            ],
          ),
        ),
      ),
    );
  }
}

Before

Screenshot 2024-09-04 at 12 38 05

After

Screenshot 2024-09-04 at 12 38 28

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages f: scrolling Viewports, list views, slivers, etc. labels Sep 2, 2024
@TahaTesser
TahaTesser marked this pull request as ready for review September 2, 2024 16:18
Comment on lines +969 to +972
final bool isIconButton = leading is IconButton
|| leading is CloseButton
|| leading is BackButton
|| leading is DrawerButton;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to detect these additional buttons use IconButton underneath?

@QuncCccccc QuncCccccc Sep 4, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor _ActionButton to extends IconButton? Seems _ActionButton just returns a IconButton

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While exploring this suggestion I realized i also missed EndDrawerButton in this check. This really goes to show this type of fix is not very robust.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the _ActionButton to extend IconButton and reverted the updated check

@QuncCccccc QuncCccccc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +969 to +972
final bool isIconButton = leading is IconButton
|| leading is CloseButton
|| leading is BackButton
|| leading is DrawerButton;

@QuncCccccc QuncCccccc Sep 4, 2024 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor _ActionButton to extends IconButton? Seems _ActionButton just returns a IconButton

@TahaTesser

Copy link
Copy Markdown
Contributor Author

@QuncCccccc this seems to be failing Google testing

@QuncCccccc

QuncCccccc commented Sep 5, 2024 •

Copy link
Copy Markdown
Contributor

@QuncCccccc this seems to be failing Google testing

I'm working on it!😎

Update: sent cl/671169268 and cl/671527109 for review

@QuncCccccc QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 6, 2024
@auto-submit
auto-submit Bot merged commit 755cf0b into flutter:master Sep 6, 2024
@TahaTesser

Copy link
Copy Markdown
Contributor Author

@QuncCccccc thanks for helping!

@TahaTesser
TahaTesser deleted the fix_m3_app_bar_leading_size branch September 6, 2024 21:22
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 8, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 12, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Material 3] Date Range Picker close button has incorrect shape AppBar back button focus/hover circle should not fill up whole height

2 participants