feat(tree-grid): add cascading row selection #8040 #8772
Conversation
…into mevtimov/feat-8040
…into mevtimov/feat-8040
mmart1n
added
grid: general
grid: row-selection
❌ status: awaiting-test
| @@ -34,6 +34,12 @@ export const GridSelectionMode = mkenum({ | |||
| }); | |||
| export type GridSelectionMode = (typeof GridSelectionMode)[keyof typeof GridSelectionMode]; | |||
|
|
|||
| export const HierarchicalGridSelectionMode = mkenum({ | |||
There was a problem hiding this comment.
Don't introduce a new enum, just add the new selection mode to the GridSelectionMode. If anyone sets it on a flat or hierarchical grid, then selection would still work as multiple, because the isMultiRowSelectionEnabled would still return true, even though there's no cascading in flat and hiearchical grids.
There was a problem hiding this comment.
Technically, the 'enum' is only there for compatibility with older code when those options were actual enums and for this specifically, it's not even needed but likely better to have for consistency. The selection property (and others we've converted) are simple string union types now - their usage in no way requires the exported constants like this one as they are not even necessary. Just like in markup, this is perfectly valid:
this.grid1.rowSelection = 'multiple';There's intellisense autocomplete, it's type checked (i.e. you can't just assign a random string) and will even validate conditions like this:
It costs us very little override the 'single' | 'multiple' | 'multipleCascade' with 'single' | 'multiple' for the flat grid and I'd rather do that to avoid the API exposing something it doesn't support even if it would technically still work. There's no impact on existing code, even for the tree grid you can still assign values from the old constant too.
Again, the exported constants are only there for backwards compatibility and/or [questionable] convenience and the selection pre-dated the migration, so the new one is there to be consistent, though for new 'enums' those constants are not even needed to be publicly exported and can be kept just for internal references.
There was a problem hiding this comment.
@damyanpetev This is unnecessary code pollution, which adds no value to the actual features. All it does for us is increase the size of the bundle. Because of the new type, now the rowSelection property needs to be introduced in each type of grid with the appropriate type of enum, which is unnecessary.
There was a problem hiding this comment.
Yes, no value added for the feature itself, the only benefit was keeping the public API of the unaffected grids clean. We were more concerned with that, rather than the duplicating one get/set (the H Grid override is supposed to be temporary). However, I see your point about pollution and duplication and I agree it's unnecessary debt and a small bump to the packed size.
Not sure if unreasonable, but the duplication can be avoided if the base is generic on the selection mode, and this I think can be implemented fairly gracefully with very little changes overall by making it optional as well. Basically, just convincing TypeScript to show the correct type, while keeping all the code the same.
If you think that's overkill, then a comment on the property the multipleCascade only applies to the Tree Grid (or doesn't apply to flat and H grids) will have to do.
| /** | ||
| * @hidden @internal | ||
| */ | ||
| public get isMultiRowSelectionEnabled(): boolean { |
There was a problem hiding this comment.
You don't need this. Leave the base implementation take care of it. It will return true on GridSelectionMode.multiple and on GridSelectionMode.multipleCascade
There was a problem hiding this comment.
Fair enough, there's very little reason to override this check indeed
| /** | ||
| * @hidden @internal | ||
| */ | ||
| public get isMultiRowSelectionEnabled(): boolean { |
There was a problem hiding this comment.
You don't need this either. Leave the base implementation take care of it. It will return true on GridSelectionMode.multiple and on GridSelectionMode.multipleCascade
| /** | ||
| * @hidden | ||
| */ | ||
| protected _rowSelectionMode: GridSelectionMode; |
There was a problem hiding this comment.
This is is overridden just so the getter doesn't complain about the return type, though (if the types are kept) this is easily adjusted in the getter as well with a type assert.
There was a problem hiding this comment.
@damyanpetev I don't want a new enum type though, and so this won't be necessary.
| @@ -260,11 +260,39 @@ export class IgxGridComponent extends IgxGridBaseDirective implements GridType, | |||
| * @hidden | |||
| */ | |||
| protected groupingDiffer; | |||
| protected _rowSelectionMode: GridSelectionMode; | |||
| }); | ||
|
|
||
| this.onRowDeleted.pipe(takeUntil(this.destroy$)).subscribe(args => { | ||
| if (this.rowSelection === 'multipleCascade') { |
There was a problem hiding this comment.
Checks should be performed against the enum, rather than against the string representation. This way we can change the string without having to find and replace it everywhere!
| @@ -360,12 +362,81 @@ export class IgxTreeGridComponent extends IgxGridBaseDirective implements GridTy | |||
| this.loadChildrenOnRowExpansion(args); | |||
| }); | |||
|
|
|||
| this.onRowAdded.pipe(takeUntil(this.destroy$)).subscribe(args => { | |||
| if (this.rowSelection === 'multipleCascade') { | |||
There was a problem hiding this comment.
Checks should be performed against the enum, rather than against the string representation. This way we can change the string without having to find and replace it everywhere!
|
|
||
| /** Select specified rows. No event is emitted. */ | ||
| selectRowsWithNoEvent(rowIDs: any[], clearPrevSelection?): void { | ||
| if (this.grid && this.grid.rowSelection === 'multipleCascade') { |
There was a problem hiding this comment.
Checks should be performed against the enum, rather than against the string representation. This way we can change the string without having to find and replace it everywhere!
|
|
||
| /** Deselect specified rows. No event is emitted. */ | ||
| deselectRowsWithNoEvent(rowIDs: any[]): void { | ||
| if (this.grid.rowSelection === 'multipleCascade') { |
There was a problem hiding this comment.
Checks should be performed against the enum, rather than against the string representation. This way we can change the string without having to find and replace it everywhere!
| } | ||
|
|
||
| public emitRowSelectionEvent(newSelection, added, removed, event?): boolean { | ||
| if (this.grid.rowSelection === 'multipleCascade') { |
There was a problem hiding this comment.
Checks should be performed against the enum, rather than against the string representation. This way we can change the string without having to find and replace it everywhere!
|
@teodosiah Can you also add an entry for the new feature in the changelog :) |
…into mevtimov/feat-8040
…teui-angular into mevtimov/feat-8040
projects/igniteui-angular/src/lib/grids/selection/selection.service.ts
Outdated
Show resolved
Hide resolved
damyanpetev
changed the title
DiyanDimitrov
added
✅ status: verified
Closes #8040
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)