Skip to content

Update NavigationBar to support Material 3 token #98285

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 6 commits into from
Feb 15, 2022
Merged

Update NavigationBar to support Material 3 token #98285

fluttergithubbot merged 6 commits into from
Feb 15, 2022

Conversation

@darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Feb 11, 2022

Part of: #91605

Updated NavigationBar widget to support Material Design 3 tokens. The NavigationBar is a Material 3 widget, but the current implementation doesn't match the latest spec and doesn't use the generated token values for defaults. This PR adds support for this.

NavigationBar

In order to not break anyone currently using this widget, the new support is gated by ThemeData.useMaterial3 flag. To use the NavigationBar with the Material 3 token defaults, turn on the useMaterial3 flag in the ThemeData:

  return MaterialApp(
    theme: ThemeData.light().copyWith(useMaterial3: true),
    // ...
  );

This also adds new elevation and indicatorShape parameters to the NavigationBarThemeData.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Feb 11, 2022
rami-a
rami-a approved these changes Feb 14, 2022
View reviewed changes
packages/flutter/lib/src/material/navigation_bar.dart Outdated
Copy link
Contributor

@rami-a rami-a Feb 14, 2022

Choose a reason for hiding this comment

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

When we were working on this with designers, I believe this component (and the app bar) were supposed to have no shadow but still receive a surface overlay. It appears the tokens do not indicate this though.

I've messaged designers to see what the deal is exactly and I'll CC you Darren.

Copy link
Contributor Author

@darrenaustin darrenaustin Feb 14, 2022

Choose a reason for hiding this comment

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

Thx. It would be nice to have this somehow encoded in the tokens. We need the elevation for the overlay calculation, so it might make sense to have a useShadow or such in the tokens that we would mirror in the Material widget.

Copy link
Contributor

@rami-a rami-a Feb 15, 2022

Choose a reason for hiding this comment

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

According to our designer, the current way this is indicated is that a .shadow-color token is present when there should be a shadow. However for the navigation bar, it is present but that is a bug. So that should get fixed in another version of the tokens. It's a bit of hacky logic however

Copy link
Contributor

@LasseRosenow LasseRosenow Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What about just changing the value of the .shadow-color token to transparent for the navigationbar? That would be way less hacky / confusing I think.

packages/flutter/lib/src/material/navigation_bar.dart Outdated
Copy link
Contributor

@rami-a rami-a Feb 14, 2022

Choose a reason for hiding this comment

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

Since this widget is still fairly new, if it is possible to remove these defaults that would make the code more straightforward (they dont actually follow any spec as they were sort of a in-between M2 and M3 thing)

Copy link
Contributor Author

@darrenaustin darrenaustin Feb 14, 2022

Choose a reason for hiding this comment

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

I wasn't sure how many people had been using this widget, so I didn't want to break anyone. I would be more than happy to just ditch the pre-M2 settings if this isn't an issue for anyone.

HansMuller
HansMuller approved these changes Feb 15, 2022
View reviewed changes
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

@fluttergithubbot fluttergithubbot merged commit ca2a751 into flutter:master Feb 15, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2022
@engine-flutter-autoroll
ca2a751 Migrate NavigationBar to M3 tokens. (flutter/flutter#98285)
d9a9bb1
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 15, 2022
@engine-flutter-autoroll
ca2a751 Migrate NavigationBar to M3 tokens. (flutter/flutter#98285)
cee618e
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 15, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 15, 2022
@engine-flutter-autoroll
ca2a751 Migrate NavigationBar to M3 tokens. (flutter/flutter#98285)
e25ee5e
@darrenaustin darrenaustin mentioned this pull request Feb 23, 2022
Closed
70 tasks
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Mar 8, 2022
@darrenaustin @clocksmith
Migrate NavigationBar to M3 tokens. (flutter#98285)
779c7ce
@guidezpl guidezpl changed the title Migrate NavigationBar to M3 tokens. Update NavigationBar to support Material 3 token Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnsonmh johnsonmh Awaiting requested review from johnsonmh

@guidezpl guidezpl Awaiting requested review from guidezpl

3 more reviewers

@rami-a rami-a rami-a approved these changes

@LasseRosenow LasseRosenow LasseRosenow left review comments

@HansMuller HansMuller HansMuller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@darrenaustin @HansMuller @rami-a @LasseRosenow @fluttergithubbot