feat(material/list): add test harnesses for list components. #17859
Conversation
googlebot
added
the
cla: yes
mmalerba
force-pushed
the
list-harness
branch
from
d4476f5 to
2288e7f
Compare
| text?: string | RegExp; | ||
| } | ||
|
|
||
| export interface ListItemHarnessFilters extends BaseListItemHarnessFilters {} |
There was a problem hiding this comment.
Any reason not to just have ListItemHarnessFilters be the base type instead of having a separate BaseListItemHarnessFilters type?
There was a problem hiding this comment.
It didn't feel right since the other types don't actually extend MatList. However, I don't really see needing to add a filter for normal list items that doesn't also apply to the other list item types, so it would probably be fine if we want to do that.
mmalerba
force-pushed
the
list-harness
branch
from
2288e7f to
81cd497
Compare
mmalerba
left a comment
mmalerba
left a comment
There was a problem hiding this comment.
Most of the comments addressed, will move the files around in a followup commit to make following the comments easier.
| export interface SelectionListHarnessFilters extends BaseHarnessFilters {} | ||
|
|
||
| export interface BaseListItemHarnessFilters extends BaseHarnessFilters { | ||
| text?: string | RegExp; |
There was a problem hiding this comment.
I don't think so. Because the list doesn't have hierarchical sections, its not always clear if an item belongs to a header or not. e.g.:
Subheader 1
Item 1
Item 2
-----------
Item 3 <-- under Subheader 1?
| text?: string | RegExp; | ||
| } | ||
|
|
||
| export interface ListItemHarnessFilters extends BaseListItemHarnessFilters {} |
There was a problem hiding this comment.
It didn't feel right since the other types don't actually extend MatList. However, I don't really see needing to add a filter for normal list items that doesn't also apply to the other list item types, so it would probably be fine if we want to do that.
mmalerba
force-pushed
the
list-harness
branch
from
81cd497 to
cbbadda
Compare
| "dialog", | ||
| "dialog/testing", | ||
| "divider", | ||
| "divider/testing", |
There was a problem hiding this comment.
It's consistent, but it feels a little silly to have a test harness for what is basically a div.
There was a problem hiding this comment.
Yeah its a little silly, but there are actually couple methods on it, and just having the selector encapsulated in a harness is kind of nice
|
|
||
| /** Harness for interacting with a list subheader. */ | ||
| export class MatSubheaderHarness extends ComponentHarness { | ||
| static hostSelector = '[mat-subheader], [matSubheader]'; |
There was a problem hiding this comment.
Aren't we running the risk of these selectors going out of sync? Maybe in a follow-up we should pull them out into variables?
There was a problem hiding this comment.
I think it'll be ok, usually when I remove a selector I do a global search for e.g. 'mat-subheader' to find and fix them all, that should catch this.
mmalerba
force-pushed
the
list-harness
branch
from
b6b9804 to
88ce469
Compare
|
Comments addressed, PTAL |
mmalerba
force-pushed
the
list-harness
branch
from
88ce469 to
2a6c91a
Compare
|
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. |
4 participants
No description provided.