feat(material-experimental/mdc-chips): add `remove` method to MDC chip harness #20831

MatteoVH · 2020-10-19T20:02:13Z

This allows MDC chips to be removed in testing environments.

Note that the harness must not only click on the remove button, but also dispatch a transitionend event. Without the dispatch of this event, the chip would not fire it's removed output.

This PR also includes a basic MDC ChipRemoveHarness that mirrors the non-MDC version. This was necessary to find the ChipRemove button in the chip harness' remove method.

MatteoVH · 2020-10-19T20:04:39Z

This works towards fixing #20826

jelbourn

Could you add a unit test for the remove functionality?

MatteoVH · 2020-10-19T20:19:14Z

Of course, looking into a unit test now.

crisbeto

Also blocked on #20764 based on the discussion in #20826.

crisbeto · 2020-10-20T15:51:34Z

src/material-experimental/mdc-chips/testing/chip-harness.ts

+    if (!isRemovable) {
+      throw new Error('This chip is not removable');
+    }
+


I think that we should implement this by dispatching a "delete" keydown event, instead of going through the remove button since a chip can be removable without having a remove button.

I'm trying it that way, but the delete doesn't actually seem to trigger correctly from a 'keydown' event. Any ideas?

I can debug more easily once this gets synced into our repo.

crisbeto · 2020-10-20T15:53:50Z

src/material-experimental/mdc-chips/testing/chip-harness.ts

+
+  /** Indicates if this chip can be deleted. */
+  async isRemovable(): Promise<boolean> {
+    const removeHarness = await this.locatorForOptional(MatChipRemoveHarness)();


The presence of a remove button doesn't determine whether a chip is removable, but rather the removable input. Is there a use case for this method, aside from throwing the error inside remove? If not, I think that we can get rid of this method since we don't have it on the non-MDC harness either.

Got it, I'll remove this method.

crisbeto · 2020-10-20T15:54:44Z

src/material-experimental/mdc-chips/testing/chip-remove-harness.ts

+import {ChipRemoveHarnessFilters} from './chip-harness-filters';
+
+/** Harness for interacting with a mat-chip removal button in tests. */
+export class MatChipRemoveHarness extends ComponentHarness {


The non-MDC harness has a click method so we should probably add it to this one too.

Sorry, that's outside the scope of what I'd like to accomplish here.

crisbeto · 2020-10-20T15:56:39Z

Also I'm not sure why the CI checks aren't running against this PR.

jelbourn · 2020-10-20T22:33:00Z

@MatteoVH you wouldn't happen to have CircleCI set up on your fork, would you?

MatteoVH · 2020-10-28T19:13:04Z

I don't believe I do, checking my github integrations on CircleCI doesn't show any.

crisbeto

The recent changes LGTM once the CI is green.

MatteoVH · 2020-10-30T20:59:20Z

Hey @crisbeto, The 'transitionend' event doesn't seem to be firing correctly when I use a delete 'keydown' vs. clicking on a MatChipRemove element. Specifically, this @HostListener isn't being triggered. Any ideas why that might be?

crisbeto · 2020-10-30T21:06:56Z

Without having dived into it, here are a couple of things that you can try:

Using TestElement.sendKeys to dispatch the keyboard event.
Adding a await this.forceStabilize() before and after the transitionend event to make sure that we've waited to all the change detection-related events have finished.

…p harness This allows MDC chips to be removed in testing environments. Note that the harness must not only click on the remove button, but also dispatch a `transitionend` event. Without the dispatch of this event, the chip would not fire it's `removed` output. This PR also includes a basic MDC ChipRemoveHarness that mirrors the non-MDC version. This was necessary to find the ChipRemove button in the chip harness' `remove` method.

MatteoVH · 2020-11-04T00:29:04Z

@crisbeto, looks like I was misspelling the event type. Should be all good now.

crisbeto

LGTM

angular-automatic-lock-bot · 2020-12-05T00:10:28Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 19, 2020

MatteoVH marked this pull request as ready for review October 19, 2020 20:04

MatteoVH requested a review from mmalerba as a code owner October 19, 2020 20:04

jelbourn requested a review from crisbeto October 19, 2020 20:14

jelbourn reviewed Oct 19, 2020

View reviewed changes

jelbourn added area: material/chips G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround labels Oct 19, 2020

MatteoVH mentioned this pull request Oct 19, 2020

Align MDC-based chip harnesses with existing MatChipHarness #20826

Closed

jelbourn added the target: minor This PR is targeted for the next minor release label Oct 19, 2020

crisbeto reviewed Oct 20, 2020

View reviewed changes

crisbeto added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Oct 20, 2020

MatteoVH force-pushed the mat-chip-harness-remove branch from eb0f2d3 to a9ffa7a Compare October 28, 2020 19:29

crisbeto reviewed Oct 30, 2020

View reviewed changes

mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Oct 30, 2020

MatteoVH changed the title ~~add remove API to MDC chip harness~~ feat(material-experimental/mdc-chips): add remove method to MDC chip harness Nov 3, 2020

MatteoVH force-pushed the mat-chip-harness-remove branch 5 times, most recently from 3f2299a to c702990 Compare November 3, 2020 23:55

MatteoVH force-pushed the mat-chip-harness-remove branch from 83a26eb to e6cb039 Compare November 4, 2020 00:01

crisbeto approved these changes Nov 4, 2020

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker merge safe labels Nov 4, 2020

annieyw merged commit d29ef76 into angular:master Nov 4, 2020

MatteoVH deleted the mat-chip-harness-remove branch November 4, 2020 22:36

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 5, 2020

feat(material-experimental/mdc-chips): add remove method to MDC chip harness #20831

feat(material-experimental/mdc-chips): add remove method to MDC chip harness #20831

Uh oh!

Conversation

MatteoVH commented Oct 19, 2020

Uh oh!

MatteoVH commented Oct 19, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

MatteoVH commented Oct 19, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

MatteoVH Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

crisbeto Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MatteoVH Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

crisbeto Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

MatteoVH Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Oct 20, 2020

Uh oh!

jelbourn commented Oct 20, 2020

Uh oh!

MatteoVH commented Oct 28, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

MatteoVH commented Oct 30, 2020

Uh oh!

crisbeto commented Oct 30, 2020

Uh oh!

MatteoVH commented Nov 4, 2020

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Dec 5, 2020

Uh oh!

Uh oh!

feat(material-experimental/mdc-chips): add `remove` method to MDC chip harness #20831

feat(material-experimental/mdc-chips): add `remove` method to MDC chip harness #20831

crisbeto Oct 20, 2020 •

edited

Loading