fix(angular): inline overlay dynamic template data #24521
Conversation
| @@ -103,7 +103,6 @@ export class IonModal { | |||
| protected el: HTMLElement; | |||
|
|
|||
| constructor(c: ChangeDetectorRef, r: ElementRef, protected z: NgZone) { | |||
| c.detach(); | |||
There was a problem hiding this comment.
Does this need to be done for inline popovers as well? https://github.com/ionic-team/ionic-framework/blob/main/angular/src/directives/overlays/popover.ts#L107
Additionally, how does this impact change detection with a controller (if it impacts it at all)?
There was a problem hiding this comment.
Good catch, this did affect popovers as well. I've updated the PR to resolve and test both. I've also reworked the commit since this is more about inline overlays and less specific to just modals.
Tested with controllers & attached change detection and things are working as expected. I cannot notice an impact.
There was a problem hiding this comment.
I was wondering why this call was added in the first place, and traced it back to the inline modal feature PR: https://github.com/ionic-team/ionic-framework/pull/23341/files#diff-d123478d8d877b48ad22b0a74610bcd649b94548e5e5c56a173bb3506d506e88R25 @liamdebeasi Can you think of why this was added originally? I'm wondering if there was some edge case bug this was meant to fix 🤔
There was a problem hiding this comment.
c.detach() is what all the other components do, which is why I added it https://github.com/ionic-team/ionic-framework/blob/main/angular/src/directives/proxies.ts#L26
There was a problem hiding this comment.
Huh... do we have our own change detection set up or something? 🤨
There was a problem hiding this comment.
How we "wrap" our web components into Angular is sorta special. We basically target the same selector tag as our web component, but then use Angular's content projection with ng-content to slot the existing web component inside the wrapping Angular component; since ng-content isn't an actual DOM node this results in ion-modal equalling our web component node.
We then typically detach from all of Angular's change detection, because the updating of the web component should be driven through our implementation in Stencil and independent to anything in Angular. We don't want to cause an unwanted repaint of the web component. We only pass through inputs from our proxy to the web component node, so that the web component can decide when a re-render is necessary.
Overlays are a special condition (inline most specifically), because we are reliant on using ng-template since we are using template outlets (Angular feature). In Ivy, they fixed ng-template to have its own change detection reference to render changes inside just that template context. When we "detach" we are opting out of that behavior. With controller implementations the custom Angular component the implementer creates; would drive the change detection of the template (which wouldn't be detached by default).
For all our other UI components, opting out makes sense. We aren't needing the benefits of functionality that Angular brings with what the web component needs.
There was a problem hiding this comment.
So that's what those are for here! Thanks for the in-depth explanation 👍 If controller modals/popovers still work as expected (i.e. no extra repaints) then this sounds good to me.
There was a problem hiding this comment.
It would be good to note this somewhere either in the Ionic wiki or in the comments in this code
There was a problem hiding this comment.
Adding a notion doc for now in our internal documents.
sean-perkins
changed the title
Allow template bindings to update with inline overlays. Resolves #24502
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Angular inline modals and popovers do not update their template contents when change detection occurs. This results in structural directives (
*ngIf,*ngFor), pipes and bindings to not update after the component is initialized.Issue Number: Resolves #24502
What is the new behavior?
Inline modals and popovers will correctly re-render their template contents when change detection occurs.
Does this introduce a breaking change?
Other information
Kapture.2022-01-05.at.22.44.57.mp4
Kapture.2022-01-05.at.22.41.40.mp4