Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(elements-grid): Add content children ready event. #15226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+55 −7
Open

feat(elements-grid): Add content children ready event. #15226

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
560f034
feat(elements-grid): Add content children ready event.
Jan 9, 2025
13053be
chore(*): Apply review comments.
Jan 14, 2025
6516dfe
chore(*): Add condition based on content children with rel. query col…
Jan 15, 2025
8b5559b
chore(*): Add scenario to test hgrid as well.
Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions projects/igniteui-angular-elements/src/app/custom-strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,22 @@ 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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, further annoying bit can be the descendants bit on the query, might need to account for that as well

for (const iterator of toBeOrphanedChildren) {
// TODO: special registration OR config for custom
}

const promises = [];
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(() => {
Copy link
Member

@damyanpetev damyanpetev Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@damyanpetev damyanpetev Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(this as any).componentRef?.instance.contentChildrenReady.emit();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

This is why I want the special event to be part of the config, so the strategy can check which components should emit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, a tag that generates a config is fine, but a null check would be simpler. Do we need a config for everything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

let parentInjector: Injector;
let parentAnchor: ViewContainerRef;
const parents: IgcNgElement[] = [];
Expand Down Expand Up @@ -211,6 +224,19 @@ class IgxCustomNgElementStrategy extends ComponentNgElementStrategy {
}
}

public waitForCondition(conditionFn: any, interval = 10) {
return new Promise<void>((resolve) => {
function checkCondition() {
if (conditionFn()) {
resolve();
} else {
setTimeout(checkCondition, interval);
}
}
checkCondition();
});
}

public override setInputValue(property: string, value: any, transform?: (value: any) => any): void {
if ((this as any).componentRef === null ||
!(this as any).componentRef.instance) {
Expand Down
9 changes: 5 additions & 4 deletions projects/igniteui-angular-elements/src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ <h3 class="ig-typography__h6">Flat Grid (MRL column layout)</h3>

grid1.data = northwindProducts;
grid2.data = northwindProducts;
grid1.addEventListener('contentChildrenReady', () => {
console.log("Ready!");
restoreState();
});
const categories = Array.from(new Set(northwindProducts.map(x => x.CategoryName)));

let groupingExpression1 = [];
Expand Down Expand Up @@ -206,10 +210,7 @@ <h3 class="ig-typography__h6">Flat Grid (MRL column layout)</h3>
document.getElementById("saveState").addEventListener("click", saveState);
document.getElementById("restoreState").addEventListener("click", restoreState);
document.getElementById("toggleIcon").addEventListener("click", toggleIcon);
const stateComponent = document.getElementById('state');
stateComponent.options = {
paging: false
};

});

buttonGroup.addEventListener("igcSelect", (e) => {
Expand Down
10 changes: 9 additions & 1 deletion projects/igniteui-angular/src/lib/grids/grid-base.directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,14 @@ export abstract class IgxGridBaseDirective implements GridType,
@Output()
public selectedRowsChange = new EventEmitter<any[]>();

/* blazorInclude */
/** @hidden @internal */
/**
* Emitted when content children are attached and collections in grid are updated.
*/
@Output()
public contentChildrenReady = new EventEmitter<void>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


/**
* Emitted when the expanded state of a row gets changed.
*
Expand Down Expand Up @@ -4930,7 +4938,7 @@ export abstract class IgxGridBaseDirective implements GridType,
* @param value
* @param condition
* @param ignoreCase
* @deprecated in version 19.0.0.
* @deprecated in version 19.0.0.
*/
public filterGlobal(value: any, condition, ignoreCase?) {
this.filteringService.filterGlobal(value, condition, ignoreCase);
Expand Down
Loading