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

FIX: https://github.com/flutter/samples/issues/1752 and https://githu… #1753

Merged
merged 4 commits into from
May 2, 2023

Conversation

rydmike
Copy link
Contributor

@rydmike rydmike commented Apr 28, 2023

…b.com//issues/1751

Fixes for:

The screen that shows Material elevation and row that says "Surface Tint Color Only" has shadow and shown elevation dp is wrong.

FIX: #1751
FIX: #1752

Pre-launch Checklist

  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I read the Contributors Guide.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-devrel channel on Discord.

@parlough
Copy link
Member

Thanks for the PR :)

Could you also make your changes to the experimental version of this app? https://github.com/flutter/samples/blob/main/experimental/material_3_demo/lib/elevation_screen.dart

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.

Hi @rydmike! Thanks a lot for the contribution! Just left one comment below:)

Also sorry if I didn't give a prompt response on other issues. I will work on them one by one!

ElevationGrid(surfaceTintColor: surfaceTint),
ElevationGrid(
surfaceTintColor: surfaceTint,
shadowColor: Colors.transparent,
Copy link
Contributor

@QuncCccccc QuncCccccc Apr 28, 2023

Choose a reason for hiding this comment

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

Edited: Originally I wanted to compare: (1) only setting surface tint color v.s. (2) setting both v.s. (3) only setting shadow color. But it might look confusing because the first two rows are identical when shadow color is null. Sorry about that!

I'm thinking, instead of setting this shadow color to be transparent, maybe we can set a different shadow color, such as colorScheme.primary, so that we can see the difference easily.
Screenshot 2023-04-28 at 3 56 03 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Just gave it another thought. I think setting it to transparent is a better way to only show the surface tint color. So LGTM! I can make the same change to the experimental one.

Copy link
Contributor Author

@rydmike rydmike Apr 29, 2023

Choose a reason for hiding this comment

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

@QuncCccccc yes I agree, since the idea is to demonstrate normal shadow+tint and only tint based elevations.

Originally this screen looked as I made it look again, the tint only row, had no shadows. Later a change in the framework of how Material behaves in M3 mode, broke the original design. This brings back the original intent to present the different forms of elevation possibilities in Material 3.

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! Thanks for the help!

ElevationGrid(surfaceTintColor: surfaceTint),
ElevationGrid(
surfaceTintColor: surfaceTint,
shadowColor: Colors.transparent,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just gave it another thought. I think setting it to transparent is a better way to only show the surface tint color. So LGTM! I can make the same change to the experimental one.

@rydmike
Copy link
Contributor Author

rydmike commented Apr 29, 2023

@parlough Sure I will update the PR to include the same fixes to the experimental version too.

EDIT: Done

Apply FIX for:
FIX: flutter#1751
FIX: flutter#1752
to experimental version of material_3_demo
@domesticmouse
Copy link
Contributor

@QuncCccccc can you please re-review for the changes to the experimental version as well?

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!

@domesticmouse
Copy link
Contributor

@rydmike can you please bring this PR up to date with main? The one PR you are behind contains the break fix that should turn most of the CI green.

@domesticmouse domesticmouse merged commit faade5f into flutter:main May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material_3_demo] Elevations show wrong elevation dp [material_3_demo] Surface Tint Color Only Has Shadow
4 participants