feat(chips): add test harness #20028
Conversation
crisbeto
added
P3
crisbeto
requested review from
devversion,
jelbourn,
mmalerba and
a team
as code owners
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
chips-harness
branch
2 times, most recently
from
f2ae6a8 to
964cdef
Compare
Adds test harnesses for the Material chips and the related components.
|
The feedback has been addressed. |
| /** Only find instances whose text matches the given value. */ | ||
| text?: string | RegExp; | ||
| /** Only find chip instances whose selected state matches the given value. */ | ||
| selected?: boolean; |
There was a problem hiding this comment.
The mdc versions of chips has types of chips that don't have a selected state- this would affect the filter as well as the chip harness API. Would it make sense to author the harness for the existing chip-list against the concepts of the MDC-based chips so that we're going in the direction we want? (e.g MatChipListbox, MatChipOption, etc.)
There was a problem hiding this comment.
Wouldn't those just always return false? I think that no matter what, we'll have some differences in the harnesses between the current ones and MDC, because the public API is different.
There was a problem hiding this comment.
IIRC the MDC-based mat-chip-listbox should be equivalent to the current mat-chip-list, so maybe the end result should look something like
MatChipHarness - any kind of chip, no selection API
MatChipSetHarness - any kind of chip container, no selection API
MatChipOptionHarness - specifically a listbox (option) chip, extends MatChipHarness, selection API
MatChipListboxHarness - specifically listbox chip container, extends MatChipSetHarness, selection API
MatChipListHarness becomes an alias for MatChipListboxHarness
Thoughts?
There was a problem hiding this comment.
IMO that makes sense for the MDC chips, but not for the existing ones. Somebody that hasn't used the MDC chips won't know what a "chip listbox" is referring to. I don't think it's worth it to avoid breaking changes in the harnesses, considering that the component's public API has breaking changes already.
There was a problem hiding this comment.
Yeah I think since one of the main goals for the harnesses is to help with the MDC migration, we should align the API with the current version of chips for now. Once the migration is done we may want to change it to align better with the new API of the MDC version.
There was a problem hiding this comment.
How would this all work with the current version of chips though? Would there would be 2 different harnesses for MatChip (MatChipHarness, MatChipOptionHarness) and users just pick the ones that gives the functionality they want in their test?
There was a problem hiding this comment.
It's worth noting that selectable is an input so a chip could change type between change detection runs. That means that people might have to use two harnesses for the same element.
There was a problem hiding this comment.
I think you could just use MatOptionHarness exclusively and it would report false if it wasn't selectable, so I don't think you'd need to use both for a single element (unless MatChipHarness does something that MatChipOptionHarness doesn't)
There was a problem hiding this comment.
That's the point I was making above about always using MatChipOption. Anyway, I don't think that we can get away with using the exact same set of harnesses for both MDC and the current version, because they're fundamentally different APIs.
There was a problem hiding this comment.
I suppose I had just been imagining that we could just completely omit the selection APIs from MatChipHarness for now, and only add them to the mdc ones.
I admit it's not ideal. Thinking about it more, I can live with going with this API for now and just committing to writing migration schematics for chips specifically because the APIs should be pretty easy to transform.
jelbourn
added
action: merge
|
(marking as not merge-safe because this touches mat-chip-list) |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds test harnesses for the Material chips and the related components.
Note: I didn't add harnesses for
MatChipAvatarandMatChipTrailingIconbecause they don't have any functionality tied to them.