Skip to content
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

md-filter-chip: remove event and its usability #5118

Open
datvm opened this issue Oct 25, 2023 · 3 comments
Open

md-filter-chip: remove event and its usability #5118

datvm opened this issue Oct 25, 2023 · 3 comments

Comments

@datvm
Copy link
Contributor

datvm commented Oct 25, 2023

Description

Hi, first of all, thanks a lot for fixing #4951 . During the last few days I have been upgrading my client websites' with the new material components and today I stumpled upon this scenario with <md-filter-chip> (and <md-input-chip> as well).

Right now the documentation does not mention any event at all which is not really helpful if you want to know when a chip get removed. I see it can be fixed with additional annotation:

 * @fires remove Fired once the chip is removed

However, another issue is when remove is fired, the actual chip is not removed yet. This causes very awkward code when handling the event where you have to consider that possibility, for example, when getting values from the children, you have to keep a reference to the chip that is being removed:

    #onChipRemoved() {
        // The removed chip is not removed yet at this time
        if (this.#lstSplitTimes.children.length <= 1) {
            this.#lstSplitTimes.append(this.#lblNoSplit);
        }
        // Code for getting values from children is impossible here without keeping a reference to the
        // chip that fired this event
    }

I understand you should not remove it yet because the user can cancel it using preventDefault. My suggestion is to give us another event, like removed so we can simply handle it if we don't care about cancelling:

function handleRemoveClick(this: Chip, event: Event) {
  if (this.disabled) {
    return;
  }

  event.stopPropagation();
  const preventDefault =
      !this.dispatchEvent(new Event('remove', {cancelable: true}));
  if (preventDefault) {
    return;
  }

  this.remove();

 // Add this:
  this.dispatchEvent(new Event('removed'));
}

P.s. I know it's discussed before but I still wish the events bubble, or at least re-dispatched by the <md-chip-set> so I don't have to add event handlers to each <md-...-chip> 😅

Browser/OS Environment

No response

@vdegenne
Copy link
Contributor

vdegenne commented Oct 25, 2023

I think it's the same problem as #4951 before.

We could remove the chip right on the click event, and revert it if the user explicitly used preventDefault by attaching the chip back where it was located, that would fix the issue.
But we are shifting the problem to users that want to prevent the default because now they would see a removed chip even tough they prevented the removal.

So I agree having a custom event e.g. removed would be way more convenient. It's not just for chips, I also faced this issue with checkboxes and switches.

A temporary solution you may be aware of is waiting the update to complete,

async #onChipRemoved() {
  await Promise.all(this.chipset.chips.map(c => c.updateComplete));
  // Changes are now reflected
  // ...
}

But I understand your concern.

@asyncLiz
Copy link
Collaborator

I think this makes sense, but we probably won't introduce it until we add chip removal animations.

The chip set is supposed to listen to the remove request, then animating the chip removing. I think the chip set should dispatch events when chips are removed or added.

@datvm
Copy link
Contributor Author

datvm commented Oct 25, 2023

@asyncLiz that's great. When you design and implement it please consider how you will dispatch it from the chip set (I assume you won't give individual chip event anymore) so we still have information of which chip is getting removed (are you providing the info through a CustomEvent.detail, what will you provide: an index, its value or the entire Element reference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants