Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues remain in parent attachment, reparenting, and row-island projection behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes nested Angular Elements grid views so they attach to parent view containers, preserve DOM order, and update correctly with zoneless change detection.
Changes:
- Replaces obsolete sink anchors with parent
ViewContainerRefattachment. - Updates DOM-position restoration and row-island projections.
- Adds view hierarchy, DOM-position, and change-detection tests.
File summaries
| File | Summary |
|---|---|
projects/igniteui-angular/grids/tree-grid/src/tree-grid.component.html |
Removes obsolete sink markup. |
projects/igniteui-angular/grids/pivot-grid/src/pivot-grid.component.html |
Removes obsolete sink markup. |
projects/igniteui-angular/grids/hierarchical-grid/src/hierarchical-grid.component.html |
Removes obsolete sink markup. |
projects/igniteui-angular/grids/grid/src/grid.component.html |
Removes obsolete sink markup. |
projects/igniteui-angular/grids/grid/src/grid-base.directive.ts |
Removes the anchor property. |
projects/igniteui-angular-elements/src/lib/grids/row-island.component.ts |
Updates projected content; a moderate visibility/overlay issue remains (1 vote). |
projects/igniteui-angular-elements/src/app/custom-strategy.ts |
Changes view attachment and DOM restoration; critical parent-attachment (2 votes), moderate reparenting (1 vote), and trailing-anchor coverage nit (1 vote) remain. |
projects/igniteui-angular-elements/src/app/custom-strategy.spec.ts |
Adds hierarchy, DOM-position, and change-detection regression tests. |
Review details
Suppressed comments (3)
projects/igniteui-angular-elements/src/app/custom-strategy.ts:140
- Once a nested element's view is inserted into this VCR, a DOM move across semantic parents within Angular Elements' delayed-destroy window only cancels the pending destroy;
connect()does not rerun this parent lookup or reinsert the view. The element therefore remains attached to the old parent's VCR/injector, so change detection from the new parent cannot reach it (and destroying the old parent can destroy the moved child). Reconcile the logical view attachment when a reconnect resolves a different parent, or do not preserve the instance across such a reparenting move.
parentAnchor = parentInjector.get(ViewContainerRef);
projects/igniteui-angular-elements/src/app/custom-strategy.ts:198
- This new reference-node logic is specifically meant to handle views whose
rootNodesinclude a trailing anchor comment (igc-action-strip/igc-grid-state), but the added DOM-position test only creates toolbar, column, and paginator elements. A regression in the reason for replacingelement.nextSiblingwould therefore still pass the suite. Please add an action-strip or grid-state fixture and assert that it remains in its authored position after attachment.
const nextSibling = (this as any).componentRef.hostView.rootNodes.at(-1).nextSibling;
projects/igniteui-angular-elements/src/lib/grids/row-island.component.ts:32
- Removing this wrapper changes more than the anchor: row-island content includes
igc-action-strip(for example,src/app/index.html:304-318), whose theme makes the host a full-size absolutely positioned overlay (action-strip/themes/_base.scss:31-50). The new template therefore renders that declarative action strip visibly at the row-island declaration instead of keeping it hidden/configuration-only, which can add an extra overlay to the grid. Keep a non-anchoringdisplay: nonewrapper around these projections while removing only#sink.
template: `
<ng-content select="igx-column,igc-column,igx-column-group,igc-column-group,igx-action-strip,igc-action-strip"></ng-content>
<ng-content select="igx-row-island,igc-row-island"></ng-content>
`,
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
damyanpetev
left a comment
There was a problem hiding this comment.
Minor nits on the tests and comments, overall LGTM;
damyanpetev
left a comment
There was a problem hiding this comment.
LGTM!
Note: This doesn't dismiss the need to resolve the toolbar actions rendering dependency on the grid running full change detection (prob move to signals), it just makes Elements consistent to Angular's runtime.
Closes #17598
Description
Nested custom elements are now attached into their parent element's view instead of being attached to the
ApplicationRefas standalone roots.IgxCustomNgElementStrategyresolves aViewContainerReffrom the parent component's injector (a container anchored at the parent's host element) and inserts the child's host view there.Because the anchor is no longer needed, the
@igxElementsAnchorplumbing is removed: theanchorproperty onIgxGridBaseDirectiveand the<div #sink>markup in the grid, tree grid, hierarchical grid, pivot grid and row island templates.The DOM position restore that follows the insert now derives its reference node from the last of the host view's root nodes rather than from
element.nextSibling.ViewContainerRef.insert()relocates every root node of the view, and some components (igc-action-strip,igc-grid-state) have a trailing anchor comment in addition to the element - using it as the reference node makesinsertBeforethrowNotFoundErroronce it has been moved.Motivation / Context
Elements runs with zoneless change detection, which only walks views reachable from whatever was marked dirty. A detached root is unreachable from its parent, so a grid's own
notifyChanges()/markForCheck()never refreshed nested elements and they kept rendering stale state until something ticked their own root.This surfaced in the grid toolbar actions, which bind directly to live grid state rather than to inputs:
igc-grid-toolbar-pinningkept showing the oldpinnedColumnsCountafter pinning/unpinning a columnigc-grid-toolbar-advanced-filteringkept showing the old column count after applying a filterThe same scenarios work under zone.js, where every tick re-checks all attached views regardless of the view hierarchy, and in a plain Angular app, where these components are genuine descendants of the grid's view.
Type of Change (check all that apply):
Component(s) / Area(s) Affected:
Grid, Tree Grid, Hierarchical Grid, Pivot Grid
How Has This Been Tested?
Three tests added to
custom-strategy.spec.ts, all verified to fail when the behaviour is reverted:ApplicationRefroot views; toolbar, title, actions, hiding, pinning, column and paginator are not.pinnedColumnsCountin the nested toolbar action, with no input or event reaching that element.Test Configuration:
Screenshots / Recordings
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)