fix(grids): any columns in the template should not override the autogenerated ones [Elements] - 18.1.x

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

571 stars 162 forks source link

fix(grids): any columns in the template should not override the autogenerated ones [Elements] - 18.1.x #14590

Closed ddaribo closed 2 months ago

ddaribo commented 3 months ago

Closes #14260

Additional information (check all that apply):

[x] Bug fix
[ ] New functionality
[ ] Documentation
[ ] Demos
[ ] CI/CD

Checklist:

[x] All relevant tags have been applied to this PR
[x] This PR includes unit tests covering all the new code (test guidelines)
[ ] This PR includes API docs for newly added methods/properties (api docs guidelines)
[ ] This PR includes feature/README.MD updates for the feature docs
[ ] This PR includes general feature table updates in the root README.MD
[ ] This PR includes CHANGELOG.MD updates for newly added functionality
[ ] This PR contains breaking changes
[ ] This PR includes ng update migrations for the breaking changes (migrations guidelines)
[ ] This PR includes behavioral changes and the feature specification has been updated with them

damyanpetev commented 3 months ago

Isn't this a bit of mis-configuration in the first place? What's the point of declaring a column only to expect it to be removed?

If it's a bug fix, ideally I'd like to see a test - either in Angular or Elements-specific if it can't be reproduced, though it should be. With Elements it's all about timing with child elements: The declared columns (due to the custom components handling) are not instantly (well on first content init) populated like in the direct Angular runtime and run a dozen ms later. That could cause the data to be set first and generate columns and the static column to be added later. Should be easy to reproduce w/ igx- components by simply declaring the child column with a condition that you enable after the grid initializes.

Though seeing it that way the fix might be introducing some unintended behavior for Angular and might not belong in the main source or not worth the fix in the first place.

dkamburov commented 2 months ago

Yes, Damyan is completely right, just tested it with Angular and if you apply some further changes to templates of the columns after initial render, they do get applied.

  <button (click)="toggle = !toggle">click</button>
  <igx-grid #grid [data]="data" [autoGenerate]="true">
    <igx-column [field]="'ID'" dataType="string" [sortable]="true"></igx-column>
    <igx-column *ngIf="toggle" [field]="'ContactName'" dataType="string"></igx-column>
  </igx-grid>

Not sure why this remark was added in the first place and I see why the confusion. But this is a scenario we don't really what to go handle.

The users should either use [autoGenerate]=true or define columns through template, not both. The fix is adding additional handling therefore complexity for a scenario with not well specified behavior.

ddaribo commented 2 months ago

@dkamburov, I see, thanks for the input! In that case I will be closing the PRs and the issue.