[Chip] Add ripple when clickable #17829
Conversation
Details of bundle changes.Comparing: 5d564f9...5e4abf3
|
oliviertassinari
added
the
PR: needs revision
|
Hey @oliviertassinari, |
oliviertassinari
added
the
component: chip
|
@mbrookes Could you throw some light on this issue ? |
|
You're heading in the right direction. The MD spec didn't have a ripple for the chip when this component was first created, but I'm glad to see that it was added since. One thing to consider is under what circumstances a clicked chip should ripple. At the moment the Chip behaves differently depending on whether an onClick (or onDelete) function is provided. (This is a little different to other components, for better or worse...) You could do the same for the ripple, rather than having it always ripple when clicked. |
@mbrookes Interesting. For the multi-select case, we need the chip to be focusable and to have a delete button. Vuetify can be used to benchmark: https://vuetifyjs.com/en/components/chips#chips. Would the following work?
|
That's already supported. The ripple on click is incidental to that use case. |
|
@mbrookes Agree, chip for multi-select is perfect right now, I don't want to have a regression :) |
|
I made the changes, but tests are still failing due. |
mbrookes
changed the title
oliviertassinari
removed
the
PR: needs revision
oliviertassinari
added
new feature
oliviertassinari
changed the title
|
thanks for updating the tests @oliviertassinari |
|
Should the clickable chip ripple when the delete icon is clicked? |
I would say no, but what's the use case for a clickable chip with a delete icon? |
|
It's in the spec:
They don't show a specific example, but I would think something like an expandable chip that can also be deleted? |
|
@mbrookes I have no solution to propose 🤔 |
Giving more thought to this problem, I would be fine if we keep the ripple: we have customization demos where the delete icon is replaced with an end leading icon. I would propose to get developers feedback before changing the behavior. |
In the Chip playground? Perhaps a checkmark is not the best choice as an alternate delete icon? In any case, I would suggest that if we can, we support the default use-case out-of-the-box, and wait for developer feedback on the need for an alternative. 🙂 |
|
I meant the checked icon in the very first demo: Ok, so it's good to go, as is? |
Oops, was too busy looking for a customisation demo!
I don't think the chip should ripple when the delete icon is clicked. However, it may not be a problem in practice if the deleted chip is immediately removed from the DOM. In other words, it only appears "wrong" in our static demos, because the chip isn't removed when delete is clicked. The caveat here is that in the Chip spec under Behaviour / Transformative, the example image appears to show the added chip having a grow transition. This could be taken to imply that a deleted chip should have the reverse. In that case, a flash of ripple might be undesirable. On balance, I think it's okay to merge as is, and wait to see if it's considered a problem. |
|
I think clicking/focusing the delete button inside the chip should not bubble up i.e. stop propagating the events. Then the whole chip should stop rippling. This also makes more sense since you probably don't want the chip click handler to be executed when delete is clicked. |
|
I would be eager to collect more user feedback on this. From a product perspective, do we have enough context? The stop propagation could be a valid solution. I guess we would need to stop mousedown, mouseup, touchstart, touchend, & onclick. |
|
@Tarun047 Thank you! |
Updated Chip Component to use Button Base.
This enables the Repl Effect for the Chip component.
Fixes #17718