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 token usages on Regular Chip and Action Chip #141701
Fix token usages on Regular Chip and Action Chip #141701
Conversation
… in this type of chip
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the contribution! Just left some questions below:)
@@ -60,10 +64,10 @@ class _${blockName}DefaultsM3 extends ChipThemeData { | |||
Color? get surfaceTintColor => ${colorOrTransparent("$tokenGroup.container.surface-tint-layer.color")}; | |||
|
|||
@override | |||
Color? get checkmarkColor => ${color("$tokenGroup.with-icon.selected.icon.color")}; | |||
Color? get checkmarkColor => null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we here use "$tokenGroup.with-icon.icon.color"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuncCccccc I've written my thoughts on this in the main thread: #141701 (comment)
Let me know what you think
|
||
@override | ||
Color? get deleteIconColor => ${color("$tokenGroup.with-icon.selected.icon.color")}; | ||
Color? get deleteIconColor => null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -10,7 +10,7 @@ class ChipTemplate extends TokenTemplate { | |||
super.textThemePrefix = '_textTheme.' | |||
}); | |||
|
|||
static const String tokenGroup = 'md.comp.assist-chip'; | |||
static const String tokenGroup = 'md.comp.filter-chip'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment in the original issue, this change makes sense to me. Maybe @TahaTesser has more context about this:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since chip_template
is for generic chip, using filter chip tokens to get most tokens available makes sense.
@@ -41,21 +45,23 @@ class _${blockName}DefaultsM3 extends ChipThemeData { | |||
Color? get surfaceTintColor => ${colorOrTransparent("$tokenGroup.container.surface-tint-layer.color")}; | |||
|
|||
@override | |||
Color? get checkmarkColor => ${color("$tokenGroup.with-icon.selected.icon.color")}; | |||
Color? get checkmarkColor => null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we change tokenGroup to filterChip, should we here use with-leading-icon.selected.leading-icon.color
?
@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.
But it would be great to read some other opinion 😄 |
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Thanks for the explanation! That makes sense to me:) LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Same here. Doing a local test for G3 before we merge this in master. |
Could you help solve the conflicts:)? |
It should be good now. The conflict was in |
…6028) Manual roll Flutter from c65ab4d513da to e02e2079bea7 (38 revisions) Manual roll requested by stuartmorgan@google.com flutter/flutter@c65ab4d...e02e207 2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from f4fbabf1eb9f to 68943afd62d1 (9 revisions) (flutter/flutter#142690) 2024-02-01 36861262+QuncCccccc@users.noreply.github.com Introduce tone-based surfaces and accent color add-ons - Part 1 (flutter/flutter#142654) 2024-02-01 andrewrkolos@gmail.com improve error message when `--base-href` argument does not start with `/` (flutter/flutter#142667) 2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from c4247c5e31ba to f4fbabf1eb9f (1 revision) (flutter/flutter#142675) 2024-02-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from c83617eee093 to c4247c5e31ba (3 revisions) (flutter/flutter#142662) 2024-02-01 jonahwilliams@google.com [Impeller] opt vulkan tests into GPU tracing. (flutter/flutter#142649) 2024-02-01 gspencergoog@users.noreply.github.com Convert button `.icon` and `.tonalIcon` constructors to take nullable icons. (flutter/flutter#142644) 2024-02-01 davidmartos96@gmail.com Fix token usages on Regular Chip and Action Chip (flutter/flutter#141701) 2024-02-01 hans.muller@gmail.com Added ButtonStyle.foregroundBuilder and ButtonStyle.backgroundBuilder (flutter/flutter#141818) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from 5b89189b8b5f to c83617eee093 (2 revisions) (flutter/flutter#142656) 2024-01-31 christopherfujino@gmail.com [flutter_tools] add debugging to ios/core_devices.dart (flutter/flutter#142187) 2024-01-31 gspencergoog@users.noreply.github.com 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 engine-flutter-autoroll@skia.org Roll Flutter Engine from 20e53614c16c to 5b89189b8b5f (2 revisions) (flutter/flutter#142640) 2024-01-31 dnfield@google.com Refactor ShaderTarget to not explicitly mention impeller or Skia (flutter/flutter#141460) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9ccd81d7595b to 20e53614c16c (3 revisions) (flutter/flutter#142628) 2024-01-31 louisehsu@google.com Show Mac Designed For iPad in 'flutter devices' (flutter/flutter#141718) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_arm64_ios basic_material_app_ios__compile to be unflaky (flutter/flutter#142594) 2024-01-31 goderbauer@google.com Fix ParentDataWidget crash for multi view scenarios (flutter/flutter#142486) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from e0d8f472a1b6 to 9ccd81d7595b (1 revision) (flutter/flutter#142625) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_arm64 tool_tests_commands to be unflaky (flutter/flutter#142593) 2024-01-31 jmccandless@google.com "System back gesture" explanation (flutter/flutter#142254) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_x64 tool_tests_commands to be unflaky (flutter/flutter#142592) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_x64_ios integration_test_test_ios to be unflaky (flutter/flutter#142595) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_x64 native_ui_tests_macos to be unflaky (flutter/flutter#142598) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_x64_ios hot_mode_dev_cycle_ios__benchmark to be unflaky (flutter/flutter#142597) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_arm64 native_ui_tests_macos to be unflaky (flutter/flutter#142599) 2024-01-31 fluttergithubbot@gmail.com Marks Windows_android hot_mode_dev_cycle_win__benchmark to be flaky (flutter/flutter#142609) 2024-01-31 fluttergithubbot@gmail.com Marks Mac_arm64_ios integration_test_test_ios to be unflaky (flutter/flutter#142596) 2024-01-31 polinach@google.com Mark test that leaks image. (flutter/flutter#142539) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from b9bc256156b8 to e0d8f472a1b6 (1 revision) (flutter/flutter#142623) 2024-01-31 31859944+LongCatIsLooong@users.noreply.github.com Fix unresponsive mouse tooltip (flutter/flutter#142282) 2024-01-31 fluttergithubbot@gmail.com Marks Linux_android_emu android_defines_test to be unflaky (flutter/flutter#142591) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from 447dd212447e to b9bc256156b8 (6 revisions) (flutter/flutter#142617) 2024-01-31 engine-flutter-autoroll@skia.org Roll Packages from 25abb5d to 5b48c44 (4 revisions) (flutter/flutter#142616) 2024-01-31 64037520+SelaseKay@users.noreply.github.com Fix null operator error when tapping on 'MenuItemButton' (flutter/flutter#142230) 2024-01-31 engine-flutter-autoroll@skia.org Roll Flutter Engine from 8e7df85f7d11 to 447dd212447e (2 revisions) (flutter/flutter#142587) 2024-01-31 katelovett@google.com 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 rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. ...
The regular chip and the action chip templates were referencing non existent M3 design tokens.
Fixes #141288
The
ActionChip
doesn't have any visual difference. Even though the template and file changes, the defaultlabelStyle
color already usesonSurface
.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 thefilter-chip
spec as described in the linked issue.Before
After
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.