-
Notifications
You must be signed in to change notification settings - Fork 160
IgxGrid disabled row implementation. #7104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ps://github.com/IgniteUI/igniteui-angular into revert-6972-mkirova/row-pinning-export-to-excel
…a/row-pinning-export-to-excel
…g-export-to-excel Revert "Row pinning + export to excel"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just noted a few potential improvements.
@@ -5137,7 +5156,7 @@ export class IgxGridBaseDirective extends DisplayDensityBase implements | |||
* ``` | |||
*/ | |||
get dataView(): any[] { | |||
return this.verticalScrollContainer.igxForOf; | |||
return this.verticalScrollContainer.igxForOf.map(rec => this.isGhostRecord(rec) ? rec.recordRef : rec) ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mapping still necessary? We don't extract the original data from meta data records for other features (like group records, hierarchical records etc.) so unless there is some specific integration scenario that depends on the value to be extracted this is probably not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally I suspect this getter is called quite a lot around the grid code/templates. With a sufficiently big data, this is basically creating and discarding a new array on each call.
@@ -89,10 +89,14 @@ export class IgxGridHierarchicalRowPinning implements PipeTransform { | |||
|
|||
constructor(private gridAPI: GridBaseAPIService<IgxHierarchicalGridComponent>) { } | |||
|
|||
public transform(collection: any[], pinnedArea: boolean, pipeTrigger: number): any[] { | |||
public transform(collection: any[], isPinned: boolean, pipeTrigger: number): any[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As base grid pinning pipe and the one for the hierarchical grid are the same, we can remove the one for hierarchical grid pinning and re-use the base grid one.
…Remove obsolete code and set upinned records in pipe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Closes: #6926
#6640
Additional information (check all that apply):
Checklist:
feature/README.MD
updates for the feature docsREADME.MD
CHANGELOG.MD
updates for newly added functionalityng update
migrations for the breaking changesIntegration scenarios with ghost rows:
These should be tested.