feat(chip): trailing remove icon can now be customized #5263

vdegenne · 2023-12-06T16:34:58Z

I made a preliminary commit just to show the course it's taking.

A slot was inserted directly into the renderRemoveButton method so everything works as intended with the keydown events and focus logic.

Another advantage is that md-input-chip will also inherit this change so users can use a different remove trailing icon if they want.

asyncLiz · 2023-12-06T21:53:13Z

Also let's check in on the motivation for this change. Do we want to replace the remove button's icon with something else, like a trash can? Or do we want to replace remove functionality with something new, such as a dropdown icon?

vdegenne · 2023-12-07T08:45:39Z

@asyncLiz It seems that the more general the better in this case, I see some cases where one developer would want to do something more than just removing or showing a dropdown icon (for instance showing an "info" icon that opens a small dialog to give more info about a special type of filter)

asyncLiz · 2023-12-07T18:22:19Z

I agree! Let's tackle that separately then, and scope this to just customizing the remove button's icon :)

I'll update the pr title to reflect

asyncLiz

omg I forgot to publish my comments... so sorry!

chips/internal/filter-chip.ts

chips/internal/multi-action-chip.ts

chips/internal/trailing-icons.ts

vdegenne · 2023-12-08T13:29:26Z

@asyncLiz Ok so if I understand you want to give the ability to change remove icon and later add another slot so users can insert anything at the end?
That means this code

<md-filter-chip
  removable
>
  <md-icon-button slot="trailing-icon">...</md-icon-button>
</md-filter-chip>

will end up showing two different trailing elements, is that ok?

Just to make sure we are on the same page, the modifications I provide allow this writing:

<!-- show "remove" trailing icon button by default -->
<md-filter-chip
  has-trailing
  @remove=${/* executed when remove icon button clicked */}
></md-filter-chip>

<!-- user can change the default -->
<md-filter-chip
  has-trailing
  @remove=${/* won't get executed */}
>
  <md-icon slot="trailing-icon">arrow_drop_down</md-icon>
</md-filter-chip>

We can then allow users to insert what they want in the chip by providing a general slot later on (for instance if they want to include md-menu when arrow down is clicked.)

asyncLiz · 2023-12-08T18:18:17Z

will end up showing two different trailing elements

I'm thinking that removable will not have an effect if a user provides a new trailing action.

<!-- Only the new icon button shows, not a second one. `removable` has no effect -->
<md-filter-chip removable>
  <md-icon-button slot="trailing-icon">...</md-icon-button>
</md-filter-chip>

<!-- Remove button shows with different icon -->
<md-filter-chip removable>
  <md-icon slot="remove-trailing-icon">trash</md-icon>
</md-filter-chip>

<!-- Only the new icon button shows, custom remove trailing icon does not. -->
<md-filter-chip removable>
  <md-icon slot="remove-trailing-icon">trash</md-icon>
  <md-icon-button slot="trailing-icon">...</md-icon-button>
</md-filter-chip>

asyncLiz · 2023-12-08T18:21:03Z

Just to make sure we are on the same page, the modifications I provide allow this writing:

<!-- show "remove" trailing icon button by default -->
<md-filter-chip
  has-trailing
  @remove=${/* executed when remove icon button clicked */}
></md-filter-chip>

<!-- user can change the default -->
<md-filter-chip
  has-trailing
  @remove=${/* won't get executed */}
>
  <md-icon slot="trailing-icon">arrow_drop_down</md-icon>
</md-filter-chip>

That is correct, the remove event will not fire with an entirely new custom trailing-icon slot. It would fire with a remove-trailing-icon slot.

vdegenne · 2023-12-08T19:27:55Z

@asyncLiz Last attempt. I don't want to bother I just studied the code thoroughly and tried different solutions, just making sure you fully understand this approach.
I renamed removable to has-trailing, has-trailing here basically serve as a switch for "show the trailing icon" which default to the "remove" icon, but users can then use trailing-icon to change the appearance of the icon.
get hasSlottedTrailingIcon() is used to determine if a slot content was provided but only during click event which won't compromise SSR. But ideally why not just dispatching a global event instead so in the end we could end up with a more intuitive syntax, e.g.

<md-filter-chip
  has-trailing
  @trailing-click=${this.#openChipMenu}
>
    <md-icon slot="trailing-icon">arrow_drop_down</md-icon>
    <!-- default slot -->
    <md-menu>...</md-menu>
</md-filter-chip>

Another advantage for this syntax is all the focus-ring navigation logic you wrote still act as intended and do not need additional code.

asyncLiz · 2023-12-08T19:49:59Z

We use has-* attributes specifically for SSR workarounds, so it'd be nice to keep that consistent rather than adding functionality with one of them. In general, most users shouldn't need to add has-* attributes unless they start seeing a FOUC. One day we'll remove them entirely once CSS adds :has-slotted().

I'd prefer to keep removable so it's more explicit to users what feature they're adding. It's also a breaking change if we remove it, and we want to stay on and support 1.x for a while.

I think an event like @trailing-click and attribute like trailing-icon-action or a slot="trailing-icon-button" is likely the right approach, but I'd like some more planning before we add it. For now, let's scope this PR to just customizing the existing remove button's icon with a slot="remove-trailing-icon" so we can get it in faster.

vdegenne · 2023-12-09T10:14:19Z

@asyncLiz Ok it's clear now, thanks for the heads-up. I'll complete this PR and probably leave the trailing-icon slot part to you because I'm not comfortable with the focus and aria logic yet.

asyncLiz

Looking great! Just a couple nits

asyncLiz · 2023-12-11T18:08:37Z

chips/internal/_trailing-icon.scss

-  [name="trailing-icon"]::slotted(md-icon) {
+  [name="remove-trailing-icon"]::slotted(md-icon) {
     --md-icon-size: var(--_icon-size);
     color: inherit;
  }


We should be able to remove all of this CSS by creating a wrapper <span class="trailing icon"> around the slot. Then it will re-use shared .icon styles to size the icon:

.icon ::slotted(:first-child) { font-size: var(--_icon-size); height: var(--_icon-size); width: var(--_icon-size); }

color inherits through slots, and <md-icon> explicitly has color: inherit so I don't think we need that declaration either.

Nice, I just realize font-size on md-icon will size the icon to match this value, this is so perfect.

asyncLiz · 2023-12-11T18:11:09Z

chips/internal/trailing-icons.ts

@@ -34,7 +34,7 @@ export function renderRemoveButton({
      @focus=${focusListener}>
      <md-focus-ring part="trailing-focus-ring"></md-focus-ring>
      <md-ripple ?disabled=${disabled}></md-ripple>
-      <slot name="trailing-icon" class="trailing icon">
+      <slot name="remove-trailing-icon" class="trailing icon">


Can we move the .trailing.icon to a wrapping <span> element?

<span class="trailing icon"> <slot name="remove-trailing-icon">...</slot> </span>

This will re-use the shared .icon and .icon ::slotted() styles and set things up for the next <slot name="trailing-icon">.

asyncLiz · 2023-12-11T18:13:17Z

chips/internal/trailing-icons.ts

@@ -9,7 +9,7 @@ import '../../ripple/ripple.js';

 import {html, nothing} from 'lit';

-import {Chip} from './chip.js';
+import {MultiActionChip} from './multi-action-chip.js';


We can revert this change

vdegenne · 2023-12-12T08:56:26Z

Something worth bringing to your attention, when using a custom remove icon using close value to emulate the default appearance, results look different:

(top: default, bottom: using close icon)

asyncLiz · 2023-12-12T18:34:28Z

Something worth bringing to your attention, when using a custom remove icon using close value to emulate the default appearance, results look different:

(top: default, bottom: using close icon)

Can you file a separate issue for that?

asyncLiz · 2023-12-12T18:35:12Z

chips/internal/trailing-icons.ts

-        <path
-          d="m249 849-42-42 231-231-231-231 42-42 231 231 231-231 42 42-231 231 231 231-42 42-231-231-231 231Z" />
-      </svg>
+      <span class="trailing icon">


this change is 🤌

\ o / I like how it reverted to just that

vdegenne added 2 commits December 6, 2023 16:57

chore(md-chip): filter chip trailing icon can now be customized

a4bfc96

chore(md-chip): filter chip trailing icon can now be customized

9ff29ac

vdegenne changed the title ~~Md filter chip icons~~ chore(md-chip): filter chip trailing icon can now be customized Dec 6, 2023

add hasSlottedTrailingIcon

b30d09e

asyncLiz changed the title ~~chore(md-chip): filter chip trailing icon can now be customized~~ feat(chip): filter chip trailing icon can now be customized Dec 6, 2023

asyncLiz changed the title ~~feat(chip): filter chip trailing icon can now be customized~~ feat(chip): trailing remove icon can now be customized Dec 7, 2023

asyncLiz requested changes Dec 7, 2023

View reviewed changes

chips/internal/filter-chip.ts Outdated Show resolved Hide resolved

chips/internal/multi-action-chip.ts Outdated Show resolved Hide resolved

chips/internal/trailing-icons.ts Outdated Show resolved Hide resolved

vdegenne requested a review from asyncLiz December 8, 2023 13:41

vdegenne added 2 commits December 9, 2023 11:24

Merge branch 'main' into md-filter-chip-icons

2a4db32

feat(chip): trailing remove icon can now be customized

76883cd

asyncLiz requested changes Dec 11, 2023

View reviewed changes

feat(chip): trailing remove icon can now be customized

49a6be1

vdegenne requested a review from asyncLiz December 12, 2023 08:46

feat(chip): trailing remove icon can now be customized

b44b90c

asyncLiz reviewed Dec 12, 2023

View reviewed changes

asyncLiz approved these changes Dec 12, 2023

View reviewed changes

asyncLiz added the Ready for Google label Dec 12, 2023

copybara-service bot merged commit 27d3541 into material-components:main Dec 12, 2023
3 checks passed

vdegenne deleted the md-filter-chip-icons branch December 12, 2023 20:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chip): trailing remove icon can now be customized #5263

feat(chip): trailing remove icon can now be customized #5263

vdegenne commented Dec 6, 2023 •

edited

Loading

asyncLiz commented Dec 6, 2023

vdegenne commented Dec 7, 2023

asyncLiz commented Dec 7, 2023

asyncLiz left a comment

vdegenne commented Dec 8, 2023 •

edited

Loading

asyncLiz commented Dec 8, 2023

asyncLiz commented Dec 8, 2023

vdegenne commented Dec 8, 2023 •

edited

Loading

asyncLiz commented Dec 8, 2023

vdegenne commented Dec 9, 2023

asyncLiz left a comment

asyncLiz Dec 11, 2023

vdegenne Dec 12, 2023

asyncLiz Dec 11, 2023

asyncLiz Dec 11, 2023

vdegenne commented Dec 12, 2023

asyncLiz commented Dec 12, 2023

asyncLiz Dec 12, 2023

vdegenne Dec 12, 2023

feat(chip): trailing remove icon can now be customized #5263

feat(chip): trailing remove icon can now be customized #5263

Conversation

vdegenne commented Dec 6, 2023 • edited Loading

asyncLiz commented Dec 6, 2023

vdegenne commented Dec 7, 2023

asyncLiz commented Dec 7, 2023

asyncLiz left a comment

Choose a reason for hiding this comment

vdegenne commented Dec 8, 2023 • edited Loading

asyncLiz commented Dec 8, 2023

asyncLiz commented Dec 8, 2023

vdegenne commented Dec 8, 2023 • edited Loading

asyncLiz commented Dec 8, 2023

vdegenne commented Dec 9, 2023

asyncLiz left a comment

Choose a reason for hiding this comment

asyncLiz Dec 11, 2023

Choose a reason for hiding this comment

vdegenne Dec 12, 2023

Choose a reason for hiding this comment

asyncLiz Dec 11, 2023

Choose a reason for hiding this comment

asyncLiz Dec 11, 2023

Choose a reason for hiding this comment

vdegenne commented Dec 12, 2023

asyncLiz commented Dec 12, 2023

asyncLiz Dec 12, 2023

Choose a reason for hiding this comment

vdegenne Dec 12, 2023

Choose a reason for hiding this comment

vdegenne commented Dec 6, 2023 •

edited

Loading

vdegenne commented Dec 8, 2023 •

edited

Loading

vdegenne commented Dec 8, 2023 •

edited

Loading