Skip to content

Commit

Permalink
Fix token usages on Regular Chip and Action Chip (#141701)
Browse files Browse the repository at this point in the history
The regular chip and the action chip templates were referencing non existent M3 design tokens.

Fixes flutter/flutter#141288

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

![image](https://github.com/flutter/flutter/assets/22084723/d602ef42-625a-4b5c-b63b-c46cb2070d80)

## After

![image](https://github.com/flutter/flutter/assets/22084723/dddb754f-fd29-4c4c-96cc-e7f508219f12)
  • Loading branch information
davidmartos96 authored Feb 1, 2024
1 parent ff6c8f5 commit b34ee07
Show file tree
Hide file tree
Showing 7 changed files with 40 additions and 18 deletions.
2 changes: 2 additions & 0 deletions dev/tools/gen_defaults/generated/used_tokens.csv
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Versions used, v0_162, v0_202, v0_158
md.comp.assist-chip.container.shape,
md.comp.assist-chip.container.surface-tint-layer.color,
md.comp.assist-chip.disabled.label-text.color,
md.comp.assist-chip.elevated.container.elevation,
md.comp.assist-chip.elevated.container.shadow-color,
md.comp.assist-chip.elevated.disabled.container.color,
Expand All @@ -12,6 +13,7 @@ md.comp.assist-chip.flat.disabled.outline.color,
md.comp.assist-chip.flat.disabled.outline.opacity,
md.comp.assist-chip.flat.outline.color,
md.comp.assist-chip.flat.outline.width,
md.comp.assist-chip.label-text.color,
md.comp.assist-chip.label-text.text-style,
md.comp.assist-chip.with-icon.disabled.icon.color,
md.comp.assist-chip.with-icon.icon.color,
Expand Down
10 changes: 7 additions & 3 deletions dev/tools/gen_defaults/lib/action_chip_template.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@ class _${blockName}DefaultsM3 extends ChipThemeData {
double? get pressElevation => ${elevation("$tokenGroup$elevatedVariant.pressed.container")};
@override
TextStyle? get labelStyle => ${textStyle("$tokenGroup.label-text")};
TextStyle? get labelStyle => ${textStyle("$tokenGroup.label-text")}?.copyWith(
color: isEnabled
? ${color("$tokenGroup.label-text.color")}
: ${color("$tokenGroup.disabled.label-text.color")},
);
@override
MaterialStateProperty<Color?>? get color =>
Expand All @@ -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;
@override
Color? get deleteIconColor => ${color("$tokenGroup.with-icon.selected.icon.color")};
Color? get deleteIconColor => null;
@override
BorderSide? get side => _chipVariant == _ChipVariant.flat
Expand Down
22 changes: 14 additions & 8 deletions dev/tools/gen_defaults/lib/chip_template.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
static const String variant = '.flat';

@override
Expand All @@ -29,7 +29,11 @@ class _${blockName}DefaultsM3 extends ChipThemeData {
late final TextTheme _textTheme = Theme.of(context).textTheme;
@override
TextStyle? get labelStyle => ${textStyle("$tokenGroup.label-text")};
TextStyle? get labelStyle => ${textStyle("$tokenGroup.label-text")}?.copyWith(
color: isEnabled
? ${color("$tokenGroup.unselected.label-text.color")}
: ${color("$tokenGroup.disabled.label-text.color")},
);
@override
MaterialStateProperty<Color?>? get color => null; // Subclasses override this getter
Expand All @@ -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;
@override
Color? get deleteIconColor => ${color("$tokenGroup.with-icon.selected.icon.color")};
Color? get deleteIconColor => isEnabled
? ${color("$tokenGroup.with-trailing-icon.unselected.trailing-icon.color")}
: ${color("$tokenGroup.with-trailing-icon.disabled.trailing-icon.color")};
@override
BorderSide? get side => isEnabled
? ${border('$tokenGroup$variant.outline')}
: ${border('$tokenGroup$variant.disabled.outline')};
? ${border('$tokenGroup$variant.unselected.outline')}
: ${border('$tokenGroup$variant.disabled.unselected.outline')};
@override
IconThemeData? get iconTheme => IconThemeData(
color: isEnabled
? ${color("$tokenGroup.with-icon.icon.color")}
: ${color("$tokenGroup.with-icon.disabled.icon.color")},
? ${color("$tokenGroup.with-leading-icon.unselected.leading-icon.color")}
: ${color("$tokenGroup.with-leading-icon.disabled.leading-icon.color")},
size: ${getToken("$tokenGroup.with-icon.icon.size")},
);
Expand Down
6 changes: 5 additions & 1 deletion packages/flutter/lib/src/material/action_chip.dart
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,11 @@ class _ActionChipDefaultsM3 extends ChipThemeData {
double? get pressElevation => 1.0;

@override
TextStyle? get labelStyle => _textTheme.labelLarge;
TextStyle? get labelStyle => _textTheme.labelLarge?.copyWith(
color: isEnabled
? _colors.onSurface
: _colors.onSurface,
);

@override
MaterialStateProperty<Color?>? get color =>
Expand Down
10 changes: 8 additions & 2 deletions packages/flutter/lib/src/material/chip.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2279,7 +2279,11 @@ class _ChipDefaultsM3 extends ChipThemeData {
late final TextTheme _textTheme = Theme.of(context).textTheme;

@override
TextStyle? get labelStyle => _textTheme.labelLarge;
TextStyle? get labelStyle => _textTheme.labelLarge?.copyWith(
color: isEnabled
? _colors.onSurfaceVariant
: _colors.onSurface,
);

@override
MaterialStateProperty<Color?>? get color => null; // Subclasses override this getter
Expand All @@ -2294,7 +2298,9 @@ class _ChipDefaultsM3 extends ChipThemeData {
Color? get checkmarkColor => null;

@override
Color? get deleteIconColor => null;
Color? get deleteIconColor => isEnabled
? _colors.onSurfaceVariant
: _colors.onSurface;

@override
BorderSide? get side => isEnabled
Expand Down
4 changes: 2 additions & 2 deletions packages/flutter/test/material/action_chip_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ void main() {
// Test default label style.
expect(
getLabelStyle(tester, label).style.color!.value,
theme.textTheme.labelLarge!.color!.value,
theme.colorScheme.onSurface.value,
);

Material chipMaterial = getMaterial(tester);
Expand Down Expand Up @@ -229,7 +229,7 @@ void main() {
// Test default label style.
expect(
getLabelStyle(tester, label).style.color!.value,
theme.textTheme.labelLarge!.color!.value,
theme.colorScheme.onSurface.value,
);

Material chipMaterial = getMaterial(tester);
Expand Down
4 changes: 2 additions & 2 deletions packages/flutter/test/material/chip_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ void main() {
expect(getIconData(tester).size, 18);

TextStyle labelStyle = getLabelStyle(tester, 'Chip A').style;
expect(labelStyle.color, textTheme.labelLarge?.color);
expect(labelStyle.color, lightTheme.colorScheme.onSurfaceVariant);
expect(labelStyle.fontFamily, textTheme.labelLarge?.fontFamily);
expect(labelStyle.fontFamilyFallback, textTheme.labelLarge?.fontFamilyFallback);
expect(labelStyle.fontFeatures, textTheme.labelLarge?.fontFeatures);
Expand Down Expand Up @@ -361,7 +361,7 @@ void main() {
expect(getIconData(tester).size, 18);

labelStyle = getLabelStyle(tester, 'Chip A').style;
expect(labelStyle.color, textTheme.labelLarge?.color);
expect(labelStyle.color, darkTheme.colorScheme.onSurfaceVariant);
expect(labelStyle.fontFamily, textTheme.labelLarge?.fontFamily);
expect(labelStyle.fontFamilyFallback, textTheme.labelLarge?.fontFamilyFallback);
expect(labelStyle.fontFeatures, textTheme.labelLarge?.fontFeatures);
Expand Down

0 comments on commit b34ee07

Please sign in to comment.