Skip to content

Conversation

@darrenaustin
Copy link
Contributor

@darrenaustin darrenaustin commented Jul 4, 2019

Description

In dark themes, drop shadows used to indicate elevation in Material apps can be difficult to see. To help with this, the Material Dark theme specification, adds a new semi-transparent overlay on surfaces to lighten them according to their elevation. This PR adds support for this feature.

mio-design_assets_1A1Uf3a9TsbtgSKT-hjIAaEiEIXFDTUfE_darktheme-elevationsystem

If a Material widget is in a theme where the new ThemeData.applyElevationOverlay flag is turned on, and the Material.color is the theme's surface color (i.e. ThemeData.colorScheme.surface), then a semi-transparent white will be composited over the Material.color for the background of the widget. The level of the transparency follows the values in the specification.

By default this feature is turned off to maintain backwards compatibility with apps built before the Dark theme spec was published.

To turn it on for your app, you can adjust your dark themes to have ThemeData.applyElevationOverlay set to true.

Related Issues

#35494 - A previous attempt at this PR.

Tests

I added a group of new tests in material_test.dart to exercise this feature.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

… Also

added a `darkThemeOverlay` boolean to turn it off in cases where it not
wanted.
- Renamed Material.darkThemeOverlay -> Material.applyDarkThemeElevationOverlay
- Added applyDarkThemeElevationOverlay to ThemeData
- Modified Material to use the theme's overlay setting if none is provided
  to the widget.
- Turned the overlay off by default
@darrenaustin darrenaustin added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 4, 2019
@darrenaustin darrenaustin requested review from HansMuller and rami-a July 4, 2019 00:23
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, mostly just some comments about the docs.

Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM!

- Removed applyDarkThemeElevationOverlay from Material as it is covered
  by ThemeData.
- Updated documentation based on feedback.
- Made just the overlay color animate to get around issues with
  OutlineButton.
@darrenaustin
Copy link
Contributor Author

The indentation looks off here

D'oh. Old habits die hard. Thanks.

- Only apply the overlays to Material widgets using the surface color.
- Switched to using the elevation->opacity formula that Android is using.
- Renamed applyDarkThemeElevationOverlay -> applyElevationOverlay.
- No longer checking against ThemeData.brightness to dark as
  applyElevationOverlay should only be set to true on dark themes.
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM!

),
child: buildMaterial(color: surfaceColor, elevation: 8.0))
);
final RenderPhysicalShape model = getShadow(tester);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the helper function is called getShadow? I was a little confused by this

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

// indicate the level of elevation.
Color _elevationOverlayColor(BuildContext context, Color background, double elevation) {
final ThemeData theme = Theme.of(context);
if (elevation > 0.0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Conventional formatting for this would be one-line or:

  if (elevation > 0.0 &&
      theme.applyElevationOverlay &&
      background == theme.colorScheme.surface) {

ChipThemeData chipTheme,
TargetPlatform platform,
MaterialTapTargetSize materialTapTargetSize,
bool applyElevationOverlay,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe applyElevationOverlayColor?

///
/// Material drop shadows can be difficult to see in a dark theme, so the
/// elevation of a surface should be portrayed with an "overlay" in addition
/// to the shadow. As the elevation of the component increases, the bright
Copy link
Contributor

Choose a reason for hiding this comment

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

the bright => the white

/// Note: this setting is here to maintain backwards compatibility with
/// apps that were built before the Material Dark theme specification
/// was published. New apps should set this to [true] for dark themes
/// (where [brightness] is [Brightness.dark].
Copy link
Contributor

Choose a reason for hiding this comment

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

(where => where

///
/// See also:
///
/// * [Material.elevation], which effects how transparent the overlay is.
Copy link
Contributor

Choose a reason for hiding this comment

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

the overlay => the white overlay

/// See also:
///
/// * [Material.elevation], which effects how transparent the overlay is.
/// * [Material.color], which if it matches [colorScheme.surface] an overlay
Copy link
Contributor

Choose a reason for hiding this comment

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

  • [Material.color], the white color overlay will only be applied of the material's color is [colorScheme.surface].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much clearer. Thx.

),
)
);
await tester
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be one line

- Minor formatting and doc changes
- Renamed ThemeData.applyElevationOverlay -> ThemeData.applyElevationOverlayColor
@darrenaustin darrenaustin merged commit e17f8d3 into flutter:master Jul 16, 2019
@darrenaustin darrenaustin deleted the material_dark_elevation_overlay branch July 16, 2019 17:51
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
…widget (flutter#35560)

Added support for a semi-transparent white overlay color for `Material` widgets to indicate their elevation in a dart theme. A new `ThemeData.applyElevationOverlayColor` flag was added to control this behavior, which is off by default for backwards compatibility reasons.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants