Migrated `Checkbox` to Material 3 Colors by QuncCccccc · Pull Request #110537 · flutter/flutter

QuncCccccc · 2022-08-29T21:35:54Z

Updated checkbox colors so they use the material 3 palette.

In order to use the Checkbox with the new Material 3 defaults, turn on the useMaterial3 flag in the ThemeData:

  return MaterialApp(
    theme: ThemeData(useMaterial3: true),
    // ...
  );

#104446

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
All existing and new tests are passing.

HansMuller

It seems possible that the implementation of this widget could be made substantially simpler in a future PR. Perhaps the code that computes "effective" colors by adding a state to the widgets actual state, could be deferred. So that the painter only computed those colors when it needed them. The division of labor between the Toggleable classes and this class seems like more overhead than should be needed.

HansMuller · 2022-08-29T22:41:37Z

+      if (states.contains(MaterialState.pressed)) {
+        return ${componentColor('md.comp.checkbox.unselected.pressed.state-layer')};
+      }
+      return null;


CC @darrenaustin
Should this be Colors.transparent?

HansMuller · 2022-08-29T22:42:30Z

-      ?? checkboxTheme.fillColor?.resolve(inactiveStates)
-      ?? _defaultFillColor.resolve(inactiveStates);
+    final Color? activeColor = widget.fillColor?.resolve(activeStates)
+        ?? _widgetFillColor.resolve(activeStates)


two-space indent here and below

HansMuller · 2022-08-29T22:58:35Z

+        if (states.contains(MaterialState.hovered)) {
+          return ${componentColor('md.comp.checkbox.selected.hover.container')};
+        }
+        if (states.contains(MaterialState.pressed)) {


Should the pressed test come first (here and elsewhere)? When pressed becomes true, hovered will also always be true, and so we'll miss this if statement.

Fixed. Thanks for catching this!

guidezpl

A couple of TODOs

CheckboxThemeData.side needs to support MaterialStateProperty<Side> instead of Side
Error states

Are you planning on addressing these in this PR or a future one?

guidezpl

One more thing, the useMaterial3 documentation should be updated

QuncCccccc · 2022-08-30T19:39:32Z

A couple of TODOs

CheckboxThemeData.side needs to support MaterialStateProperty<Side> instead of Side

Error states

Are you planning on addressing these in this PR or a future one?

I will create a separate PR for the TODOs:) Thanks!

…rialStateProperty method

HansMuller

LGTM

HansMuller · 2022-08-31T16:22:05Z

+        return _colors.onSurface.withOpacity(0.38);
+      }
+      if (states.contains(MaterialState.selected)) {
+        if (states.contains(MaterialState.pressed)) {


TODO (future PR): update the code generator to collapse code like this.

guidezpl

Edited the description to not close the issue, pending the remaining work.

)

QuncCccccc added 3 commits August 29, 2022 13:36

Updated Checkbox component

0145371

Removed extra commented code

c3e7214

Reformatted code

b568ee1

flutter-dashboard Bot added p: material_ui material_ui package in flutter/packages framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 29, 2022

Removed trailing space

fa30f30

QuncCccccc marked this pull request as ready for review August 29, 2022 22:33

QuncCccccc requested review from HansMuller, darrenaustin and guidezpl August 29, 2022 22:33

HansMuller reviewed Aug 29, 2022

View reviewed changes

guidezpl reviewed Aug 30, 2022

View reviewed changes

Comment thread packages/flutter/lib/src/material/checkbox.dart Outdated

guidezpl reviewed Aug 30, 2022

View reviewed changes

QuncCccccc added 2 commits August 30, 2022 13:20

addressed comments

c665acb

Corrected the order of pressed, hovered, and focused when handle Mate…

c2e977a

…rialStateProperty method

HansMuller approved these changes Aug 31, 2022

View reviewed changes

guidezpl approved these changes Aug 31, 2022

View reviewed changes

QuncCccccc changed the title ~~Migrated Checkbox to Material 3~~ Migrated Checkbox to Material 3 Colors Aug 31, 2022

Fixed commnets

2db2c9a

QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2022

auto-submit Bot merged commit a490a6a into flutter:master Aug 31, 2022

This was referenced Sep 1, 2022

Roll Flutter from f66a09791920 to 759d0e1b20c1 (56 revisions) flutter/packages#2541

Merged

Roll Flutter from 069f5042973d to 759d0e1b20c1 (16 revisions) flutter-team-archive/plugins#6349

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 1, 2022

a490a6a Migrated Checkbox to Material 3 Colors (flutter/flutter#110537

53c6c14

)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 1, 2022

a490a6a Migrated Checkbox to Material 3 Colors (flutter/flutter#110537

b8fa342

)

darrenaustin mentioned this pull request Sep 2, 2022

☂️ Bring Material 3 to Flutter #91605

Closed

63 of 70 tasks

Navigation Menu

Repository navigation

Migrated `Checkbox` to Material 3 Colors - #110537

Migrated `Checkbox` to Material 3 Colors#110537
auto-submit[bot] merged 7 commits into
flutter:masterfrom
QuncCccccc:update_checkbox_colors

QuncCccccc commented Aug 29, 2022 •

edited by guidezpl

Loading

Uh oh!

HansMuller left a comment

Uh oh!

HansMuller Aug 29, 2022

Uh oh!

HansMuller Aug 29, 2022

Uh oh!

QuncCccccc Aug 30, 2022

Uh oh!

HansMuller Aug 29, 2022

Uh oh!

QuncCccccc Aug 30, 2022

Uh oh!

guidezpl left a comment

Uh oh!

Uh oh!

guidezpl left a comment

Uh oh!

QuncCccccc commented Aug 30, 2022

Uh oh!

HansMuller left a comment

Uh oh!

HansMuller Aug 31, 2022

Uh oh!

Uh oh!

guidezpl left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

QuncCccccc commented Aug 29, 2022 • edited by guidezpl Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

HansMuller Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

HansMuller Aug 29, 2022

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

guidezpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guidezpl left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc commented Aug 30, 2022

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

guidezpl left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

QuncCccccc commented Aug 29, 2022 •

edited by guidezpl

Loading