fix(elements): Do not destroy elements inside tmpl wrapper on detach. #15678
Conversation
MayaKirova
added
❌ status: awaiting-test
tishko0
added
✅ status: verified
damyanpetev
left a comment
damyanpetev
left a comment
There was a problem hiding this comment.
@MayaKirova @skrustev @dkamburov
Just in general - don't consider fixes without the appropriate test complete or ready for review, unless the issue can't be reproduced in one. This one definitely can and I'll add it this time, but keep in mind for future PRs :)
| export class TemplateWrapperComponent { | ||
|
|
||
| public templateFunctions: TemplateFunction[] = []; | ||
| public templateRendered = new EventEmitter<any>(); |
There was a problem hiding this comment.
The correct type would be:
| public templateRendered = new EventEmitter<any>(); | |
| public templateRendered = new EventEmitter<HTMLElement>(); |
and the typing adjustments following this
There was a problem hiding this comment.
Also subject, cuz not a real Angular component with output so no benefit, just overhead.
| this.templateWrapper.templateRendered.subscribe((element) => { | ||
| const igComponents = this.config.flatMap(x => [ | ||
| x.selector, | ||
| reflectComponentType(x.component).selector |
There was a problem hiding this comment.
Pretty sure you don't need the actual reflected component selectors (i.e. the true Angular igx- components). Actually, those DOM elements won't even have the ngElementStrategy prop to adjust and shouldn't need this patch anyway :)
There was a problem hiding this comment.
Also, this is applied in general and templateRendered can be pretty intensive, so anything to reduce it (like not doing a flat map or not doing the map here at all) would be nice.
There was a problem hiding this comment.
Eh, was hoping there might be a cleaner way to make a connection between the templated and parent components, but I'll take this for now. Still, might be worth checking out if we can attach the template element to the lifecycle of the EmbeddedViewRef that created it, so we can distinguish between cached scenarios, though most that make sense (details and HGrid paginator/toolbar are).
Closes #
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)