feat(material/tree): Add test harness for MatTree
#20018
Conversation
googlebot
added
the
cla: yes
| } | ||
|
|
||
| /** Gets the tree node's text. */ | ||
| async getText(): Promise<string> { |
There was a problem hiding this comment.
I think for the nested tree node this will get the text of the node and everything under it, you might want to have separate implementations of this method for the different subclasses
| } | ||
|
|
||
| /** Gets all of the nodes in the tree. */ | ||
| async getNodes(filter: TreeNodeHarnessFilters = {}): Promise<MatTreeNodeHarness[]> { |
There was a problem hiding this comment.
@jelbourn @andrewseguin thoughts on whether we should have 2 methods or just a single getNodes method regardless of the type of node? It feels to me like we should just have 1 method
There was a problem hiding this comment.
Having these two methods does seem a bit confusing. If we can guarantee that we can operate the same API for both types of node, I think it's fine to combine them under one method.
| } from '@angular/material/tree/testing'; | ||
|
|
||
| /** Harness for interacting with a standard mat-tree in tests. */ | ||
| export class MatTreeHarness extends ComponentHarness { |
There was a problem hiding this comment.
We might want to add an additional method to get the text as some type of structured data. I think @andrewseguin did something similar for table
There was a problem hiding this comment.
I could see (in a follow-up) adding a harness method here like prettyPrint() that just prints out the text of the tree in a human-readable form for debugging. Table doesn't have something like this, but maybe it could.
The table harness (implemented by Kristiyan) does have convenience method for getting cell text strings, which we could do here too (also in a follow-up)
| } from '@angular/material/tree/testing'; | ||
|
|
||
| /** Harness for interacting with a standard mat-tree in tests. */ | ||
| export class MatTreeHarness extends ComponentHarness { |
There was a problem hiding this comment.
This could maybe be a followup PR, but I think both the tree and the node harness should implement HarnessLoader
separte out toggle method, change role to isLeaf
| /** The selector of the host element of a `MatTreeNode` instance. */ | ||
| static hostSelector = '.mat-tree-node'; | ||
|
|
||
| _toggle = this.locatorForOptional('[matTreeNodeToggle]'); |
There was a problem hiding this comment.
FWIW the intent is that this directive is just one way someone could add a toggle for a node. We may want to make the harness more flexible in terms of how expand/collapse works.
| } else if (role === 'treeitem') { | ||
| return true | ||
| } else { | ||
| throw new Error('Invalid node role'); |
There was a problem hiding this comment.
| throw new Error('Invalid node role'); | |
| throw Error('Invalid node role'); |
You don't actually need new with Error (this is different from code written inside Google where new is required just for consistency)
| /** Whether the node is a leaf node. */ | ||
| async isLeaf(): Promise<boolean> { | ||
| const role = await (await this.host()).getAttribute('role'); | ||
| if (role === 'group') { |
There was a problem hiding this comment.
FYI the way we use role="group" now is probably wrong and they should all be treeitem (related #17818).
| } | ||
|
|
||
| /** Gets the level of the tree node.. */ | ||
| async getLevel(): Promise<number> { |
There was a problem hiding this comment.
We should be clear about whether this is zero-based or one-based (see comments on #17818)
| } from '@angular/material/tree/testing'; | ||
|
|
||
| /** Harness for interacting with a standard mat-tree in tests. */ | ||
| export class MatTreeHarness extends ComponentHarness { |
There was a problem hiding this comment.
I could see (in a follow-up) adding a harness method here like prettyPrint() that just prints out the text of the tree in a human-readable form for debugging. Table doesn't have something like this, but maybe it could.
The table harness (implemented by Kristiyan) does have convenience method for getting cell text strings, which we could do here too (also in a follow-up)
| } | ||
|
|
||
| /** Gets all of the nodes in the tree. */ | ||
| async getNodes(filter: TreeNodeHarnessFilters = {}): Promise<MatTreeNodeHarness[]> { |
There was a problem hiding this comment.
Having these two methods does seem a bit confusing. If we can guarantee that we can operate the same API for both types of node, I think it's fine to combine them under one method.
|
Also the commit message prefix should be |
annieyw
changed the title
MatTreeMatTree
|
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. |
No description provided.