Fix token usages on Regular Chip and Action Chip #141701

davidmartos96 · 2024-01-17T17:56:37Z

The regular chip and the action chip templates were referencing non existent M3 design tokens.

The ActionChip doesn't have any visual difference. Even though the template and file changes, the default labelStyle color already uses onSurface.
For the reviewer, I've changed the action_chip_test to expect a color from the colorScheme so that it is more explicit that the color might not be the same as the labelLarge default in the global textTheme, even if for this case the color is the same.

The regular Chip does have visual differences, in particular, the label and trailing icon colors, which were not following the specification. In order to fix this, the regular chip now is based from the filter-chip spec as described in the linked issue.

Before

After

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.

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

…pects)

… in this type of chip

flutter-dashboard · 2024-01-17T18:33:13Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #141701 at sha 6907182

QuncCccccc

Thanks so much for the contribution! Just left some questions below:)

QuncCccccc · 2024-01-19T23:14:24Z

dev/tools/gen_defaults/lib/action_chip_template.dart


  @override
-  Color? get checkmarkColor => ${color("$tokenGroup.with-icon.selected.icon.color")};
+  Color? get checkmarkColor => null;


Should we here use "$tokenGroup.with-icon.icon.color"?

@QuncCccccc I've written my thoughts on this in the main thread: #141701 (comment)
Let me know what you think

QuncCccccc · 2024-01-19T23:14:35Z

dev/tools/gen_defaults/lib/action_chip_template.dart


  @override
-  Color? get deleteIconColor => ${color("$tokenGroup.with-icon.selected.icon.color")};
+  Color? get deleteIconColor => null;


QuncCccccc · 2024-01-19T23:33:17Z

dev/tools/gen_defaults/lib/chip_template.dart

  });

-  static const String tokenGroup = 'md.comp.assist-chip';
+  static const String tokenGroup = 'md.comp.filter-chip';


Based on the comment in the original issue, this change makes sense to me. Maybe @TahaTesser has more context about this:)

Since chip_template is for generic chip, using filter chip tokens to get most tokens available makes sense.

QuncCccccc · 2024-01-19T23:34:43Z

dev/tools/gen_defaults/lib/chip_template.dart


  @override
-  Color? get checkmarkColor => ${color("$tokenGroup.with-icon.selected.icon.color")};
+  Color? get checkmarkColor => null;


If we change tokenGroup to filterChip, should we here use with-leading-icon.selected.leading-icon.color?

davidmartos96 · 2024-01-19T23:51:36Z

@QuncCccccc Thanks for reviewing! I'll try to answer the comments here, as all of them are related.

I've purposely set those as null for a couple reasons. Described in the linked issue, but I'll try to summarize.

The checkmark and delete icons are not part of the ActionChip API, so I think it would only add confusion if we assign a token. null is giving some meaning saying there is not a token defined for those colors, as it is not in the spec.
There is already an iconTheme which applies to the leading icon.
If we were to give a color to the ActionChip delete icon, imagining it did have the API for such icon, it would be colored with the primary color (i.e. purple in the default M3 theme), instead of the the label color.

But it would be great to read some other opinion 😄

flutter-dashboard · 2024-01-29T10:28:27Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #141701 at sha 5cd04a5

QuncCccccc · 2024-01-29T18:06:54Z

@QuncCccccc Thanks for reviewing! I'll try to answer the comments here, as all of them are related.

I've purposely set those as null for a couple reasons. Described in the linked issue, but I'll try to summarize.

The checkmark and delete icons are not part of the ActionChip API, so I think it would only add confusion if we assign a token. null is giving some meaning saying there is not a token defined for those colors, as it is not in the spec.

There is already an iconTheme which applies to the leading icon.

If we were to give a color to the ActionChip delete icon, imagining it did have the API for such icon, it would be colored with the primary color (i.e. purple in the default M3 theme), instead of the the label color.

But it would be great to read some other opinion 😄

Thanks for the explanation! That makes sense to me:) LGTM.

QuncCccccc

LGTM

QuncCccccc · 2024-01-30T17:37:29Z

Same here. Doing a local test for G3 before we merge this in master.

QuncCccccc · 2024-01-31T00:29:27Z

Could you help solve the conflicts:)?

davidmartos96 · 2024-01-31T09:10:30Z

Could you help solve the conflicts:)?

It should be good now. The conflict was in used_tokens.csv, so it only was neccessary to run gen_defaults again

…tter#141701)

Manual roll Flutter from c65ab4d to e02e207 (38 revisions) Manual roll requested by [email protected] flutter/flutter@c65ab4d...e02e207 2024-02-01 [email protected] Roll Flutter Engine from f4fbabf1eb9f to 68943afd62d1 (9 revisions) (flutter/flutter#142690) 2024-02-01 [email protected] Introduce tone-based surfaces and accent color add-ons - Part 1 (flutter/flutter#142654) 2024-02-01 [email protected] improve error message when `--base-href` argument does not start with `/` (flutter/flutter#142667) 2024-02-01 [email protected] Roll Flutter Engine from c4247c5e31ba to f4fbabf1eb9f (1 revision) (flutter/flutter#142675) 2024-02-01 [email protected] Roll Flutter Engine from c83617eee093 to c4247c5e31ba (3 revisions) (flutter/flutter#142662) 2024-02-01 [email protected] [Impeller] opt vulkan tests into GPU tracing. (flutter/flutter#142649) 2024-02-01 [email protected] Convert button `.icon` and `.tonalIcon` constructors to take nullable icons. (flutter/flutter#142644) 2024-02-01 [email protected] Fix token usages on Regular Chip and Action Chip (flutter/flutter#141701) 2024-02-01 [email protected] Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder (flutter/flutter#141818) 2024-01-31 [email protected] Roll Flutter Engine from 5b89189b8b5f to c83617eee093 (2 revisions) (flutter/flutter#142656) 2024-01-31 [email protected] [flutter_tools] add debugging to ios/core_devices.dart (flutter/flutter#142187) 2024-01-31 [email protected] Fix showDialog docs (flutter/flutter#142458) 2024-01-31 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 5.0.2 to 6.0.0 (flutter/flutter#142650) 2024-01-31 [email protected] Roll Flutter Engine from 20e53614c16c to 5b89189b8b5f (2 revisions) (flutter/flutter#142640) 2024-01-31 [email protected] Refactor ShaderTarget to not explicitly mention impeller or Skia (flutter/flutter#141460) 2024-01-31 [email protected] Roll Flutter Engine from 9ccd81d7595b to 20e53614c16c (3 revisions) (flutter/flutter#142628) 2024-01-31 [email protected] Show Mac Designed For iPad in 'flutter devices' (flutter/flutter#141718) 2024-01-31 [email protected] Marks Mac_arm64_ios basic_material_app_ios__compile to be unflaky (flutter/flutter#142594) 2024-01-31 [email protected] Fix ParentDataWidget crash for multi view scenarios (flutter/flutter#142486) 2024-01-31 [email protected] Roll Flutter Engine from e0d8f472a1b6 to 9ccd81d7595b (1 revision) (flutter/flutter#142625) 2024-01-31 [email protected] Marks Mac_arm64 tool_tests_commands to be unflaky (flutter/flutter#142593) 2024-01-31 [email protected] "System back gesture" explanation (flutter/flutter#142254) 2024-01-31 [email protected] Marks Mac_x64 tool_tests_commands to be unflaky (flutter/flutter#142592) 2024-01-31 [email protected] Marks Mac_x64_ios integration_test_test_ios to be unflaky (flutter/flutter#142595) 2024-01-31 [email protected] Marks Mac_x64 native_ui_tests_macos to be unflaky (flutter/flutter#142598) 2024-01-31 [email protected] Marks Mac_x64_ios hot_mode_dev_cycle_ios__benchmark to be unflaky (flutter/flutter#142597) 2024-01-31 [email protected] Marks Mac_arm64 native_ui_tests_macos to be unflaky (flutter/flutter#142599) 2024-01-31 [email protected] Marks Windows_android hot_mode_dev_cycle_win__benchmark to be flaky (flutter/flutter#142609) 2024-01-31 [email protected] Marks Mac_arm64_ios integration_test_test_ios to be unflaky (flutter/flutter#142596) 2024-01-31 [email protected] Mark test that leaks image. (flutter/flutter#142539) 2024-01-31 [email protected] Roll Flutter Engine from b9bc256156b8 to e0d8f472a1b6 (1 revision) (flutter/flutter#142623) 2024-01-31 [email protected] Fix unresponsive mouse tooltip (flutter/flutter#142282) 2024-01-31 [email protected] Marks Linux_android_emu android_defines_test to be unflaky (flutter/flutter#142591) 2024-01-31 [email protected] Roll Flutter Engine from 447dd212447e to b9bc256156b8 (6 revisions) (flutter/flutter#142617) 2024-01-31 [email protected] Roll Packages from 25abb5d to 5b48c44 (4 revisions) (flutter/flutter#142616) 2024-01-31 [email protected] Fix null operator error when tapping on 'MenuItemButton' (flutter/flutter#142230) 2024-01-31 [email protected] Roll Flutter Engine from 8e7df85f7d11 to 447dd212447e (2 revisions) (flutter/flutter#142587) 2024-01-31 [email protected] Split out AppBar/SliverAppBar material tests (flutter/flutter#142560) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. ...

…tter#141701)

davidmartos96 added 6 commits January 17, 2024 18:27

Chip based of filter-chip instead of assist-chip

11a841b

Update ActionChip template

690d3a1

fix chip test

a8b7ce5

Update action_chip_test (no behavior change, just more clear color ex…

45e3d12

…pects)

update used_tokens

c5121eb

checkmark as null in chip_template because checkmark is not supported…

6907182

… in this type of chip

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jan 17, 2024

flutter-dashboard bot added the will affect goldens Changes to golden files label Jan 17, 2024

HansMuller requested a review from QuncCccccc January 19, 2024 22:42

QuncCccccc reviewed Jan 19, 2024

View reviewed changes

Merge branch 'master' into regular_and_action_chip

5cd04a5

QuncCccccc approved these changes Jan 29, 2024

View reviewed changes

TahaTesser approved these changes Jan 30, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into regular_and_action_chip

569cb42

QuncCccccc added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 1, 2024

auto-submit bot merged commit b34ee07 into flutter:master Feb 1, 2024

engine-flutter-autoroll mentioned this pull request Feb 1, 2024

Manual roll Flutter from c65ab4d513da to e02e2079bea7 (38 revisions) flutter/packages#6028

Merged

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

b34ee07 Fix token usages on Regular Chip and Action Chip (flutter/flu…

fdb6101

…tter#141701)

HansMuller mentioned this pull request Feb 2, 2024

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 14, 2024

b34ee07 Fix token usages on Regular Chip and Action Chip (flutter/flu…

06d87a6

…tter#141701)

davidmartos96 deleted the regular_and_action_chip branch October 13, 2024 11:09

Fix token usages on Regular Chip and Action Chip #141701

Fix token usages on Regular Chip and Action Chip #141701

Uh oh!

Conversation

davidmartos96 commented Jan 17, 2024

Before

After

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 17, 2024

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

davidmartos96 Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

TahaTesser Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jan 19, 2024

Choose a reason for hiding this comment

Uh oh!

davidmartos96 commented Jan 19, 2024

Uh oh!

flutter-dashboard bot commented Jan 29, 2024

Uh oh!

QuncCccccc commented Jan 29, 2024

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc commented Jan 30, 2024

Uh oh!

QuncCccccc commented Jan 31, 2024

Uh oh!

davidmartos96 commented Jan 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants