IgniteUI / igniteui-angular

Ignite UI for Angular is a complete library of Angular-native, Material-based Angular UI components with the fastest grids and charts, Pivot Grid, Dock Manager, Hierarchical Grid, and more.
https://www.infragistics.com/products/ignite-ui-angular
Other
569 stars 162 forks source link

[ELEMENTS] Setting template on a column on `columnInit` event in WC causes an error #13659

Open mddragnev opened 11 months ago

mddragnev commented 11 months ago

Description

Trying to set a bodyTemplate for example inside the columnInit event throws an error in WC (Blazor,React too). In the error thrown you can see that the templateRef is not a real templateRef instance but instead is the template passed. The reason is that columInit emits the actual Angular column instance but the elements grid wants to work with dom instances.

Steps to reproduce

  1. Open any WC sample
  2. Bind to the columnInit event on the grid
  3. Add a template for a column

Result

An error is thrown Screenshot 2023-11-09 at 14 10 47

Expected result

All templates boundable to the column should be able to set in columnInit event

Attachments

Attach a sample if available, and screenshots, if applicable.

github-actions[bot] commented 9 months ago

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] commented 7 months ago

There has been no recent activity and this issue has been marked inactive.

github-actions[bot] commented 4 months ago

There has been no recent activity and this issue has been marked inactive.

MayaKirova commented 4 months ago

@damyanpetev @mddragnev @dkamburov When debugging it, indeed it seem that the issue is that the columnInit emits the original angular column object, which expects a TemplateRef for the bodyTemplate. So it throw an error when it gets a function, for example if you set:

grid1.addEventListener("columnInit", (e) => {
            const detail = e.detail;
            detail.bodyTemplate = (context) => {
                return html`<div>¯\(°_o)/¯</div>`;
            };
        });

I'm not sure whether we want to support setting a callback function that returns a lit element directly on the angular column. Probably not.

We could make such events emit the igc element instead of the angular element (similar to what the wrappers do), however I'm not sure it's worth the hassle and breaking changes since the igc column could be retrieved in the event rather easily and you could set the lit function as normal, for example:

        grid1.addEventListener("columnInit", (e) => {
            const detail = e.detail;
            const targetElement = grid1.querySelector(`igc-column[field="` + detail.field +`"]`);
            targetElement.bodyTemplate = (context) => {
                return html`<div>¯\(°_o)/¯</div>`;
            }
        });

Let me know what you think.

dkamburov commented 4 months ago

I agree to that. Should I close it?

damyanpetev commented 4 months ago

I realize this can be easily worked around for this scenario and possibly in a few other cases. Still, quite odd to have to explain columnInit args can be used for most cases except the template properties and also why the typing says you're getting an IgcColumnComponent but you really don't. Some events that use the ColumnType interface could be justified if we strip the problematic props from it, but that still won't solve the problem. IIRC, that'd be the case for any other events that emit refs to components - think owner on just about anything as well the the newly-created grids from row islands. Not sure how much of the that is hit is normal use scenarios, but my point is it's not just this event. Also might not be just template props - basically anything Elements patches including wrapping method calls and sets in zone as those might not trigger proper updates.

So I still think there's something to do here - we can either try to address API types or make the results match in general

MayaKirova commented 4 months ago

@damyanpetev The typings for the columns are hardcoded in the api analyzer right now. They are the original angular's ColumnyType but are explicitly mapped to IgcColumn instead: https://github.com/IgniteUI/igniteui-api-analyzer/blob/master/src/MetadataManager.ts#L562

Perhaps we could let it generate a ColumnType so that it's easier to see that what's emitted is not an actual IgcColumn.?

damyanpetev commented 4 months ago

Like I said, that would only help if we didn't have the problematic props on those as well (we do), so both changes will likely be breaking one way or another. So still want to see if there's a general way to transform those generally, though the more I look at it the worse it gets - this affects probably a lot of events, but likely also a bunch of public API too, which will be even more annoying to handle.

PS: For now this use case is a bit of a limitation with a workaround, however not really sure of the use cases for this, outside of patching state persistence scenarios and for those the best fix remains in the state handling IMO.

MayaKirova commented 4 months ago

I believe it was logged because the workaround for restoring column templates as described in the angular docs: https://www.infragistics.com/products/ignite-ui-angular/angular/components/grid/state-persistence#restoring-columns Doesn't work the same way for wc.

But it can be documented a different way, by simply getting the igc-columns from the document:

  grid1.addEventListener("columnInit", (e) => {
            const detail = e.detail;
            const targetElement = grid1.querySelector(`igc-column[field="` + detail.field +`"]`);
            targetElement.bodyTemplate = (context) => {
                return html`<div>¯\(°_o)/¯</div>`;
            }
        });

I think that would be the best option for now.

MayaKirova commented 3 months ago

@dkamburov After investigating this some more it seems that this will work on initial load, but if the columns get re-created internally for some reason- for example if the state persistence re-creates the columns to restore some state, the newly created columns will be desynched from the igc-column in the DOM (igc-column will hold reference to an igx-column that no longer exists), so you would not be able to set anything from that point on via the igc-column instance. So this won't work for the template restoring workaround,

mddragnev commented 2 months ago

Something that we discussed with @damyanpetev is that we can try to change how the column state is processed within the angular-elements so that we dont introduce any new behaviour changes in angular. What we can do is check whether the specific column is within the grid columns with the grid public API. If not the column will be created like it is created in angular. This way if the column colection is not modified then out of the box the template would not be removed (cus the column would not be recreated) and a large percentage of use cases will be fixed.

MayaKirova commented 2 months ago

@mddragnev IMO it might be tricky to match the columns from the state's string to the actual columns, since the columns don't really have an unique identified. They have field but that's not really required or necessarily unique. You could have multiple unbound columns (no field) with templates for example and it would be difficult to match their state to the actual column instances.

github-actions[bot] commented 6 days ago

There has been no recent activity and this issue has been marked inactive.