feat(elements-grid): Add content children ready event. #15226
Conversation
MayaKirova
added
❌ status: awaiting-test
| /* blazorInclude */ | ||
| /** @hidden @internal */ | ||
| /** | ||
| * Emitted when content children are attached and collections in grid are updated. | ||
| */ | ||
| @Output() | ||
| public contentChildrenReady = new EventEmitter<void>(); |
There was a problem hiding this comment.
Was going to say we can leave this on the Elements level only, since it's only used there, but...
We could easily add this event to the static models since the Elements setup doesn't need anything extra. However, that will loose the WC type declarations from the analyzer which will actually play an important role in generating manifests we're leveraging for other platforms.
Could be done on the analyzer level, but again there are a few other things I want to address.
IMO, it'd be best if we keep this in the Elements project, though I realize how annoying it'll be to extend and override the base. Ideally, we'd just extend the declaration, however that's not available and I'm actually not sure we care to have this event on all of them - don't think the Pivot has such an issue and not sure about the row islands situation. Might be okay with extending Grids specifically in Elements.
Would also like to have some tag (like @igxChildrenReady) and have the elements analyzer add that into the config for those components so the strategy can be driven by that rather than hardcoding the emit. I could help with that part. There's also an issue that'll solve, will leave a separate comment on that.
PS: Naming TBD lol, not sure if this isn't too Angular-ish
There was a problem hiding this comment.
I'm fine with extending all the grids for elements. I think we'll eventually have to do that either way since there are many modifications made in the code specifically to make it work for the react/blazor code generations that are not relevant to angular. Tracking them all might be a pain, though. Not sure if it should be part of this feature request.
There was a problem hiding this comment.
@damyanpetev After discussing with @dkamburov I've logged this as a separate task:
#15235
All such members can be moved in the new extended component as part of that task.
| promises.push(promiseComponentReady); | ||
| } | ||
| Promise.all(promises).then(() => { | ||
| (this as any).componentRef?.instance.contentChildrenReady.emit(); |
There was a problem hiding this comment.
This will trigger for all components with content children - you can check the config but all column layouts & groups, toolbar, action strip at least. Those don't have contentChildrenReady and will actually throw here, besides it being an unnecessary work. I think the zone is doing some supper annoying consuming of promise rejections (-.-) but if you try-catch this and log the errors you'll get a bunch of those:
This is why I want the special event to be part of the config, so the strategy can check which components should emit.
There was a problem hiding this comment.
I mean, a tag that generates a config is fine, but a null check would be simpler. Do we need a config for everything?
There was a problem hiding this comment.
Oh, I sort of imagined the config having some typed section that had the content children loaded event name so that's read from there too. Possibly overcomplicating it, since the emit will be strategy code and possibly we won't care for different names, but still the event belongs to the component and would like to have as little as possible coupling.
But I'd be fine w/ just the check for this PR, sure.
| for (const contentChild of contentChildren) { | ||
| // these resolve after the parent, both the components needs to be initialized and the parent query schedule should complete before it is really ready. | ||
| const child = contentChild as IgcNgElement; | ||
| const promiseComponentReady = this.waitForCondition(() => (child.ngElementStrategy as any)?.componentRef && this.schedule.size == 0); | ||
| promises.push(promiseComponentReady); | ||
| } | ||
| Promise.all(promises).then(() => { |
There was a problem hiding this comment.
Yeah, this is less-than-ideal :) Not really a fan of the interval checking, tho need to see if anything else is viable. There's another problem too - we're not guaranteed all children will ever be registered, so a hard await on that could never complete, thought that might end up as a documented limitation of the event.
Was thinking a rolling timeout that incoming children can extend - think schedule the emit and a new child init can cancel it - I still think the event should emit when there are no children initialized after all. Not entirely sure the timing can work.
We might end up having to check if those components will be initialized and decide if we emit immediately of leave the emit at the end of updateQuery actually when the schedule empties. Should be easy enough to handle that with a flag and will skip on the additional intervals entirely. From the timing I'm seeing as long as we know updateQuery will run it's pretty much the last thing that manages to run after all children have initialized.
There was a problem hiding this comment.
If there's a child that is a valid content child of the grid and it never registers, wouldn't that be a bug rather than a valid scenario? I'm not sure in what case this would normally happen or how to even test it.
There was a problem hiding this comment.
A user error - sure, but would be nice if the event still works for the most part, sans the unregistered child element.
Otherwise, yes - not really a major concern
| @@ -66,9 +66,16 @@ class IgxCustomNgElementStrategy extends ComponentNgElementStrategy { | |||
| (this as any).componentRef = {}; | |||
|
|
|||
| const toBeOrphanedChildren = Array.from(element.children).filter(x => !this._componentFactory.ngContentSelectors.some(sel => x.matches(sel))); | |||
| const contentChildren = Array.from(element.children).filter(x => this._componentFactory.ngContentSelectors.some(sel => x.matches(sel))); | |||
There was a problem hiding this comment.
This might need some more refinement since ngContentSelectors will give all projected children and not all of those need to have a content query (e.g. the state component right now) which won't schedule any query updates and thus won't trigger the event at all.
Should be possible to further filter the children through their matching config, need the component or provideAs to match those against the componentConfig.contentQueries[n].childType-s so we know for sure a child is expected to run query update and leave the emit for that.
There was a problem hiding this comment.
Ah, further annoying bit can be the descendants bit on the query, might need to account for that as well
|
|
||
| if (contentChildren.length === 0) { | ||
| // no content children, emit event immediately, since there's nothing to be attached. | ||
| (this as any).componentRef?.instance?.childrenAttached?.emit(); |
There was a problem hiding this comment.
Also, might want to emit this after the component has actually initialized - around the end of this method.
Closes #15050
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)