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.

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

[Material] Remove opacity from dark theme overlay check #66596

Merged
merged 3 commits into from
Sep 25, 2020
Merged

[Material] Remove opacity from dark theme overlay check #66596

merged 3 commits into from
Sep 25, 2020

Conversation

johnsonmh
Copy link
Contributor

Description

This PR removes the opacities from the color check in the elevation overlay. This fixes the bug that Materials with ColorScheme.surface.withOpacity(.96) do not get the elevation overlay, even though they should.

Tests

I added the following tests:

  • Material in a dark theme app with a surface color with 75% opacity still receives the dark theme overlay.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • 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 read the Tree Hygiene wiki page, which explains my responsibilities.
  • 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

Did any tests fail when you ran them? Please read Handling breaking changes.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 24, 2020
darrenaustin
darrenaustin approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

LGTM.

packages/flutter/lib/src/material/elevation_overlay.dart
@@ -49,7 +49,7 @@ class ElevationOverlay {
if (elevation > 0.0 &&
theme.applyElevationOverlayColor &&
theme.brightness == Brightness.dark &&
color == theme.colorScheme.surface) {
color.withOpacity(1.0) == theme.colorScheme.surface.withOpacity(1.0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is getting more and more complicated. Seems like it is just a band-aid over the fact that we really don't have a concept of what is a surface and what isn't. That said, this is a good fix for now. Thx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, we are kind of patching things up. Material-Android has gotten to a stable place with dark theme logic, so hopefully once we've patched the same holes that they have, we will be able to leave it alone. Removing opacities is another part of their check.

packages/flutter/test/material/material_test.dart
@@ -343,6 +343,27 @@ void main() {
expect(model.color, equals(Colors.cyan));
});

testWidgets('overlay will apply to materials with a non-opaque surface color', (WidgetTester tester) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! 👍

perclasson
perclasson approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@perclasson perclasson left a comment

Choose a reason for hiding this comment

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

LGTM

JoseAlba
JoseAlba approved these changes Sep 25, 2020
View reviewed changes
Copy link
Contributor

@JoseAlba JoseAlba left a comment

Choose a reason for hiding this comment

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

██╗      ██████╗ ████████╗███╗   ███╗
██║     ██╔════╝ ╚══██╔══╝████╗ ████║
██║     ██║  ███╗   ██║   ██╔████╔██║
██║     ██║   ██║   ██║   ██║╚██╔╝██║
███████╗╚██████╔╝   ██║   ██║ ╚═╝ ██║
╚══════╝ ╚═════╝    ╚═╝   ╚═╝     ╚═╝

@johnsonmh johnsonmh merged commit 7d89f21 into flutter:master Sep 25, 2020
@johnsonmh johnsonmh deleted the fix-noOpacityCheckDarkTheme branch September 25, 2020 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darrenaustin darrenaustin darrenaustin approved these changes

@perclasson perclasson perclasson approved these changes

@JoseAlba JoseAlba JoseAlba approved these changes

Assignees
No one assigned
Labels
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
@johnsonmh @darrenaustin @perclasson @JoseAlba @googlebot