Skip to content

Conversation

JuYeong0413
Copy link
Contributor

@JuYeong0413 JuYeong0413 commented Sep 1, 2020

Description

Currently the CardTheme.color is ignored and ThemeData.cardColor is used in MergeableMaterial.
This pull request updates MergeableMaterial's default color to CardTheme.color.

Reproducible code sample
import 'package:flutter/material.dart';

void main() => runApp(MyApp());

class MyApp extends StatelessWidget {

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      theme: ThemeData(
        cardTheme: CardTheme(
          color: Colors.red
        ),
        cardColor: Colors.green,
      ),
      home: Scaffold(
        appBar: AppBar(
          title: Text('Flutter Demo'),
        ),
        body: SingleChildScrollView(
          child: MergeableMaterial(
            children: <MergeableMaterialItem>[
              MaterialSlice(
                key: ValueKey<String>('A'),
                child: SizedBox(
                  width: 100.0,
                  height: 100.0,
                ),
              ),
            ],
          ),
        ),
      ),
    );
  }
}
Before (ThemeData.cardColor is used as default) After (CardTheme.color is used as default) After (When CardTheme.color is not specified

Related Issues

Fixes #63398

Tests

I added the following tests:

  • mergeable_material_test.dart
    Added a test to verify that the color of MaterialSlice is applied by CardTheme.color.

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.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change.
    • I wrote a design doc:

@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 1, 2020
@AyushBherwani1998
Copy link
Member

Hey, @JuYeong0413 thank you for your contribution. This will be a breaking change and will require a breaking change proposal for the migration. Since the default color of MergableMaterial is changed it creates unexpected color changes in the existing apps. Note: MergableMaterial is also the building block for the ExpansionPanelList, so this change will also affect it.

@JuYeong0413
Copy link
Contributor Author

Thank you @AyushBherwani1998, I'll write a design doc for it ASAP.
I didn't notice that MergableMaterial is also the building block for ExpansionPanelList - should I add test for ExpansionPanelList too?

@AyushBherwani1998
Copy link
Member

Actually, @shihaohong and I have already made progress for the design doc.

The breaking change will involve 2-3 steps. One will be fixing internal google test failures if any.

If you are willing to contribute, you can hop in the second step which will be to remove the flag and making the CardTheme.color default without using a flag.

@JuYeong0413
Copy link
Contributor Author

I see. Then I'll remove the flag and make CardTheme.color as default. Thanks a lot!

@JuYeong0413 JuYeong0413 changed the title Update MergeableMaterial to respect CardTheme.color Update MergeableMaterial's default color to CardTheme.color Sep 2, 2020
@AyushBherwani1998
Copy link
Member

Sorry if it created any confusion, I didn't mean that.

You can go through the design doc. Once we have completed the first step of migration, you can hop in for the third step and contribute. We still need to work on the design doc and make a PR for the first step.

@JuYeong0413
Copy link
Contributor Author

Ahh, sorry for misunderstanding.

I would happy to contribute, but I'm little bit afraid that involving myself would slow down the progress for this change since I'm not very familiar with breaking changes. There are many ways to contribute to flutter, so maybe it would be better to close this PR and work on another issue :-) Thank you for your comments!

@JuYeong0413 JuYeong0413 closed this Sep 2, 2020
@JuYeong0413 JuYeong0413 deleted the mergeable-material branch September 2, 2020 08:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 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.

MergeableMaterial completely ignores CardTheme.color

3 participants