Skip to content

Conversation

@TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Sep 2, 2024

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. f: material design flutter/packages/flutter/material repository. 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
@TahaTesser TahaTesser force-pushed the fix_m3_app_bar_leading_size branch from 0317dac to 26e7792 Compare September 3, 2024 07:56
Comment on lines 969 to 972
final bool isIconButton = leading is IconButton
|| leading is CloseButton
|| leading is BackButton
|| leading is DrawerButton;
Copy link
Member 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?

Copy link
Contributor

@QuncCccccc QuncCccccc Sep 4, 2024

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

Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

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

@QuncCccccc QuncCccccc Sep 4, 2024

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 TahaTesser force-pushed the fix_m3_app_bar_leading_size branch from 26e7792 to 034848b Compare September 4, 2024 08:50
@TahaTesser TahaTesser force-pushed the fix_m3_app_bar_leading_size branch from c4d8555 to 7df4e32 Compare September 5, 2024 15:44
@TahaTesser
Copy link
Member Author

@QuncCccccc this seems to be failing Google testing

@QuncCccccc
Copy link
Contributor

QuncCccccc commented Sep 5, 2024

@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
Member 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: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

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