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

Revert "Add support for Material 3 Divider and VerticalDivider" #112471

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

CaseyHillers
Copy link
Contributor

@CaseyHillers CaseyHillers commented Sep 27, 2022

Reverts #112378

@TahaTesser this broke a few Google tests.

It seems like it's not pulling the correct color from the theme.

    final Color? color = this.color ?? defaults.color;

Googlers, see b/249195768

@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. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels Sep 27, 2022
@CaseyHillers CaseyHillers added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 27, 2022
@auto-submit auto-submit bot merged commit bd5c347 into master Sep 27, 2022
@auto-submit auto-submit bot deleted the revert-112378-m3_divider branch September 27, 2022 15:16
@TahaTesser
Copy link
Member

Thanks! I'll take a look.

@TahaTesser
Copy link
Member

TahaTesser commented Sep 28, 2022

@darrenaustin
Can you please check google tests? In the tests or visually, there is no difference in default M2 colors.

M2 template doesn't set any color and leaves to createBorderSide to resolve just like before this PR.

master PR

@darrenaustin
Copy link
Contributor

@darrenaustin Can you please check google tests? In the tests or visually, there is no difference in default M2 colors.

So a couple of unrelated issues. The tests that failed were golden tests that were using a divider with useMaterial3: true, but with default values. This PR changes those defaults, so the tests failed. I have worked with the team to update the app to use ThemeData.dividerTheme to ensure their dividers won't change when this lands.

The second issue was one I didn't catch in review that @CaseyHillers brought up above: we weren't using the DividerTheme.of in the color calculation (at least not in the M3 case).

I took the liberty of taking your PR and changing some of the color logic to fix this, as well as adding more of the properties to the generated theme data so that all the properties followed the same pattern. Hope you don't mind. The new PR is: #112655.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. 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.

4 participants