igxTree BiState selection#9023
Conversation
|
@mmart1n Same suggestion as to @ViktorSlavov: catch up on unit tests and continue the development using a test-driven approach. |
ViktorSlavov
left a comment
There was a problem hiding this comment.
Looks good! A couple of minor comments.
Test review still pending
| </ng-container> | ||
| </div> | ||
| <ng-container *ngIf="showSelectors"> | ||
| <div class="igx-tree-node__select-marker" (pointerdown)="$event.preventDefault()" (click)="onSelectorClick($event)"> |
There was a problem hiding this comment.
*ngIf can be here, no need for extra wrapper
| protected treeService: IgxTreeService, | ||
| protected cdr: ChangeDetectorRef, | ||
| protected builder: AnimationBuilder, | ||
| public element: ElementRef<HTMLElement>, |
There was a problem hiding this comment.
No need for this to be public
| } | ||
|
|
||
| /** | ||
| * @hidden |
There was a problem hiding this comment.
| * @hidden | |
| * @hidden @internal |
| @Injectable() | ||
| export class IgxTreeSelectionService { | ||
| public tree: IgxTreeComponent; | ||
| public nodeSelection: Set<IgxTreeNodeComponent<any>> = new Set<IgxTreeNodeComponent<any>>(); |
There was a problem hiding this comment.
Use the interface here, not the component itself, as it would be a circular dependency otherwise
| public nodeSelection: Set<IgxTreeNodeComponent<any>> = new Set<IgxTreeNodeComponent<any>>(); | |
| public nodeSelection: Set<IgxTreeNode<any>> = new Set<IgxTreeNode<any>>(); |
| export class TreeFunctions { | ||
|
|
||
| public static getAllNodes(fix) { | ||
| return fix.debugElement.queryAll(By.css('igx-tree-node')); |
There was a problem hiding this comment.
this should be constant, as the other query selectors
| <ng-container *ngTemplateOutlet="selectMarkerTemplate, context: templateContext"> | ||
| </ng-container> | ||
| </div> | ||
| <ng-container *ngIf="showSelectors"> |
There was a problem hiding this comment.
This can be moved on the div. No need for an extra wrapper element (even though ng-container is not generated).
| * | ||
| * ```typescript | ||
| * // get the nativeElement of the second node | ||
| * let nodeNativeElement = this.tree.nodes.toArray()[1].nativeElement; |
There was a problem hiding this comment.
Change this a bit, so it emphasizes on the property
| * let nodeNativeElement = this.tree.nodes.toArray()[1].nativeElement; | |
| * const node: IgxTreeNode = this.tree.nodes.first(); | |
| * const nodeElement: HTMLElement = node.nativeElement; |
| * @param nodes: IgxTreeNodeComponent<any>[] | ||
| * @param clearPrevSelection: boolean; if true clears the current selection | ||
| */ | ||
| public selectAll(nodes?: IgxTreeNodeComponent<any>[], clearPrevSelection?: boolean) { |
There was a problem hiding this comment.
Bit of a nit pick, but I'd prefer clearPrevSelection to be explicitly false instead of undefined
| public selectAll(nodes?: IgxTreeNodeComponent<any>[], clearPrevSelection?: boolean) { | |
| public selectAll(nodes?: IgxTreeNodeComponent<any>[], clearPrevSelection = false) { |
| } | ||
| } | ||
|
|
||
| public isNodeSelected(node: IgxTreeNodeComponent<any>): boolean { |
There was a problem hiding this comment.
Is this ever used? Feel like it's a leftover.
d22e523 to
409b3e2
Compare
63b8478 to
8cbbdae
Compare
6bbe579 to
b7106b9
Compare
04295af to
b686a5c
Compare
|
This is merged cause template changes are only happening here. Additional logic regarding selection is under dev in #9103 |
Closes #
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)