fix(hgrid): add and use lifecycle placeholder for grid connection eve…#17242
fix(hgrid): add and use lifecycle placeholder for grid connection eve…#17242MayaKirova wants to merge 4 commits into21.2.xfrom
Conversation
MayaKirova
added
❌ status: awaiting-test
There was a problem hiding this comment.
Pull request overview
Fixes hierarchical-grid row edit overlay desync when child grids are temporarily detached/reattached (virtualization/caching), by introducing a DOM-lifecycle “placeholder” element and wiring its connect/disconnect events to close/reopen the row edit overlay.
Changes:
- Add a native custom element (
igc-lifecycle-placeholder) that emits connect/disconnect events. - Use those events in
IgxHierarchicalGridComponentto close the row-edit overlay on detach and reopen it on reconnect. - Trigger additional view change detection during
IgxForOfDirectiveview recycling (virtualization path).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/hierarchical-grid/hierarchical-grid.sample.html |
Updates the sample row-islands’ editing configuration (adds primary keys / row editing). |
projects/igniteui-angular/grids/hierarchical-grid/src/lifecycle-placeholder-element.ts |
Introduces the custom element + registration helper for lifecycle events. |
projects/igniteui-angular/grids/hierarchical-grid/src/hierarchical-grid.component.ts |
Registers the placeholder element and adds handlers to close/reopen row edit overlay. |
projects/igniteui-angular/grids/hierarchical-grid/src/hierarchical-grid.component.html |
Renders the lifecycle placeholder element and binds to its events. |
projects/igniteui-angular/directives/src/directives/for-of/for_of.directive.ts |
Adds view.detectChanges() during view detach/insert in virtualization recycling. |
| const view = container.detach(0); | ||
|
|
||
| view.detectChanges(); | ||
| this.updateTemplateContext(embView.context, i); | ||
| container.insert(view); |
There was a problem hiding this comment.
view.detectChanges() is called before updateTemplateContext(embView.context, i), so the view will run change detection with the old context. If the goal is to refresh the recycled view immediately (and/or flush DOM connect/disconnect), update the context first and then call detectChanges() (or batch the detection outside the loop).
| const view = container.detach(container.length - 1); | ||
|
|
||
| view.detectChanges(); | ||
| this.updateTemplateContext(embView.context, i); | ||
| container.insert(view, 0); |
There was a problem hiding this comment.
Same issue here: view.detectChanges() runs before updateTemplateContext(...), so the recycled view is checked with stale $implicit/index/count values. Also, the view.detectChanges(); line is mis-indented compared to surrounding code (likely to trip formatting/lint rules).
| if (this.rowEditable && this.crudService.rowInEditMode && this.rowEditingOverlay) { | ||
| // disconnected from DOM (possibly cached) & row was in edit mode - close overlay. |
There was a problem hiding this comment.
onLifecyclePlaceholderDisconnected() calls closeRowEditingOverlay() whenever a row is in edit mode, but the overlay may already be collapsed/closed as a result of the detach that triggered this event. closeRowEditingOverlay() unconditionally touches rowEditingOverlay.element.parentElement.style, which can throw if the element is already detached (parentElement is null). Add a guard (e.g., only close when not collapsed and/or when rowEditingOverlay.element.parentElement exists) before calling closeRowEditingOverlay().
| if (this.rowEditable && this.crudService.rowInEditMode && this.rowEditingOverlay) { | |
| // disconnected from DOM (possibly cached) & row was in edit mode - close overlay. | |
| const overlay = this.rowEditingOverlay; | |
| if (this.rowEditable && this.crudService.rowInEditMode && overlay && | |
| !overlay.collapsed && overlay.element?.parentElement) { | |
| // disconnected from DOM (possibly cached) & row was in edit mode - close overlay | |
| // only if it is still open and attached. |
| // Event that triggers when element gets connected back to the DOM. | ||
| // Used to determine when to reopen a previously closed row editing overlay. | ||
| protected onLifecyclePlaceholderConnected(): void { | ||
| if (this.rowEditable && this.crudService.rowInEditMode && this.rowEditingOverlay && | ||
| this.rowEditingOverlay.collapsed) { | ||
| // Row is in edit mode, but overlay is closed - reopen. | ||
| this.openRowOverlay(this.crudService.rowInEditMode.id); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| // Event that triggers when element gets disconnected from the DOM, for example as a result of virtualization or caching. | ||
| // Used to determine when to close the row editing overlay. | ||
| protected onLifecyclePlaceholderDisconnected(): void { | ||
| if (this.rowEditable && this.crudService.rowInEditMode && this.rowEditingOverlay) { | ||
| // disconnected from DOM (possibly cached) & row was in edit mode - close overlay. | ||
| this.closeRowEditingOverlay(); | ||
| } | ||
| } |
There was a problem hiding this comment.
New connect/disconnect-driven behavior for row edit overlay state (close on disconnect, reopen on reconnect) is added here, but there are existing hgrid specs (including virtualization ones) and none appear to cover this scenario. Please add a regression test that starts row edit in a child grid, virtualizes/detaches it (scroll/collapse), and verifies the overlay is restored when reconnected.
| <igx-row-island [rowEditable]="true" [primaryKey]="'ID'" [key]="'childData'" [autoGenerate]="true" [rowSelection]='selectionMode' [batchEditing]="true" [rowEditable]="true" | ||
| [allowFiltering]="true"> | ||
| <igx-row-island [key]="'childData'" [autoGenerate]="true" [rowSelection]='selectionMode' [batchEditing]="true" [rowEditable]="true" | ||
| <igx-row-island [rowEditable]="true" [primaryKey]="'ID'" [key]="'childData'" [autoGenerate]="true" [rowSelection]='selectionMode' [batchEditing]="true" [rowEditable]="true" | ||
| [allowFiltering]="true"></igx-row-island> |
There was a problem hiding this comment.
In this sample, [rowEditable]="true" is specified twice on the same <igx-row-island> (and again on the nested one). This is redundant and can be confusing when editing the sample (and may mask accidental mismatched values). Keep a single [rowEditable] binding per element.
Hristo313
added
💥 status: in-test
Hristo313
added
✅ status: verified
…nts.
Closes #17229
Description
Add and use a native wc component as a placeholder to detect connect/disconnect events in hgrid.
Use the events to show/hide row edit overlay, since popover gets closed on detach.
Type of Change (check all that apply):
Component(s) / Area(s) Affected:
IgxHierarchicalGrid
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)