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 Material 3 medium and large top app bars. #103962

Merged
merged 3 commits into from May 20, 2022

Conversation

darrenaustin
Copy link
Contributor

A follow on to #101884.

This adds support for the new Material 3 'Medium' and 'Large' type app bars.

Two new factory methods for SliverAppBar have been added: SliverAppBar.medium and SliverAppBar.large. These can be used inside a CustomScrollView like other SliverAppBars, but will be configured for the 'Medium' or 'Large' defaults and behaviour.

Here is an example of how to create a 'Medium' app bar:

  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData(useMaterial3: true, colorSchemeSeed: const Color(0xff6750A4)),
      home: Material(
        child: CustomScrollView(
          slivers: <Widget>[
            SliverAppBar.medium(
              leading: IconButton(icon: const Icon(Icons.menu), onPressed: () {}),
              title: const Text('Medium App Bar'),
              actions: <Widget>[
                IconButton(icon: const Icon(Icons.more_vert), onPressed: () {}),
              ],
            ),

            // The rest of the scrolling slivers...

Fixes: #102684.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels May 17, 2022
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

Nice, love the screen recordings. It's unfortunate to have to redefine long argument lists to modify a few defaults. Could this be improved by collapsing the two factory constructors into one, with an additional parameter specifying the size?

examples/api/lib/material/app_bar/sliver_app_bar.3.dart Outdated Show resolved Hide resolved
examples/api/lib/material/app_bar/sliver_app_bar.3.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
@darrenaustin
Copy link
Contributor Author

darrenaustin commented May 18, 2022

It's unfortunate to have to redefine long argument lists to modify a few defaults. Could this be improved by collapsing the two factory constructors into one, with an additional parameter specifying the size?

I thought about it, but the naming would get a bit weird (i.e. what is the name of the new single factory constructor? SliverAppBar.mediumOrLarge, or SliverAppBar.scrolledUnder?). And then we need to either add an awkward mediumSize bool or introduce a new public enum with the two options just for this one parameter.

Having two constructors definitely has duplication in the parameter lists, but seems like the most straightforward and easy to use API. If people feel strongly about this, I can certainly take a stab at combining them.

@darrenaustin darrenaustin changed the title Add support for Material 3 AppBar 'Medium' and 'Large' types. Add support for Material 3 medium and large top app bars. May 18, 2022
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

Can we add Dartpad(s) like #102823?

Ignore me, just saw them :)

Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

@darrenaustin this is awesome!

@Piinks Piinks added the c: new feature Nothing broken; request for a new capability label May 19, 2022
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM, just some trivial feedback.

packages/flutter/lib/src/material/app_bar.dart Outdated Show resolved Hide resolved
@cedvdb
Copy link
Contributor

cedvdb commented May 20, 2022

It would be user friendly if the scaffold API incorporated those without having to pass through a CustomScrollView.

@darrenaustin
Copy link
Contributor Author

It would be user friendly if the scaffold API incorporated those without having to pass through a CustomScrollView.

It would be nice, but I am not sure how we could do that. The requirements for this scrolling effect are complicated enough that we need the flexibility of slivers, which requires the CustomScrollView. The Scaffold doesn't really participate in scrolling directly so I am not sure how we could better integrate these APIs.

@Piinks do you have any thoughts on this?

@Piinks
Copy link
Contributor

Piinks commented May 23, 2022

I am not sure what you mean @cedvdb, can you elaborate?

A SliverAppBar can only be used in a CustomScrollView, and I am not sure what you mean by passing through it.

The Scaffold is not aware of its contents, including the presence of a CustomScrollView. The Scaffold is already a pretty heavily loaded widget, and adding more API surface and complexity to it is likely something we would want to avoid doing.

@cedvdb
Copy link
Contributor

cedvdb commented May 23, 2022

I meant to have a SliverScaffold of some sort with factories. Imo, scenarios from the official specs could be covered as not need any CustomX. It seems out of place to use "CustomSomething" when flutter has a tight integration with material specs.

camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
…3962)

* Add support for M3 AppBar 'Medium' and 'Large' types.

* Updates from review feedback.

* Updated from review feedback.
@rydmike
Copy link
Contributor

rydmike commented Aug 23, 2022

@darrenaustin @cedvdb @Piinks @HansMuller
Is it on purpose, or an oversight that text style for _MediumScrollUnderFlexibleConfig and _LargeScrollUnderFlexibleConfig do not respect and fall through via AppBarTheme.of(context)?

They now do this:

  @override
  TextStyle? get collapsedTextStyle =>
    _textTheme.titleLarge?.apply(color: _colors.onSurface);

  @override
  TextStyle? get expandedTextStyle =>
    _textTheme.headlineSmall?.apply(color: _colors.onSurface);

I would expect them to fall through via AppBarTheme.of(context).foregroundColor to respect and use themed AppBar foreground color, like the vanilla AppBar does.

If I for example make an AppBarTheme with some blue background and white foreground, it works great on AppBar, but themed foreground color is ignored by these new sliver versions SliverAppBar.medium and SliverAppBar.large using _MediumScrollUnderFlexibleConfig and _LargeScrollUnderFlexibleConfig, while the themed background is used, as shown here:

Screen Shot 2022-08-23 at 18 52 58

Plus for it to work with Material 2 mode color defaults, it would need to replicate the standard un-themed AppBar M2 foreground color behavior as well. But OK, I can give that a pass as long as it at least respects the given AppBarTheme.


EDIT: SliverAppBar.medium and SliverAppBar.large do not even respect and use widget property passed in foregroundColor.

If things reported in this comment are seen as something that should be fixed, and not a "feature", I can open it as an issue report as well.

@HansMuller
Copy link
Contributor

@darrenaustin - #103962 (comment) looks like it covers some legitimate bugs in the new variants; please file issues

@rydmike
Copy link
Contributor

rydmike commented Sep 4, 2022

@HansMuller and @darrenaustin for info, I opened an issue for it here: #110951

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: new feature Nothing broken; request for a new capability d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update AppBar to support new M3 'Medium' and 'Large' variants.
6 participants