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

Thin abstraction over grid #4966

Closed Lightw3ight closed 4 years ago

Lightw3ight commented 5 years ago

Is your feature request related to a problem? Please describe.

My company is adopting Ignite Grid for Angular to use inside its aps, this will be the third grid component we have moved to in as many years. Every time we choose a new grid, we are left with any implementations of the old grids that need to be migrated to the new grid. For this reason, we want to create an abstraction around ignite grid, to safe guard us in the future.

Describe the solution you'd like

We have started creating a thin abstraction over the ignite grid and the template of our grid looks a little like this

<igx-grid [data]="data">
    <ng-container *ngFor="let col of columns">
        <ng-container *ngTemplateOutlet="templateChooser; context: { col: col }"></ng-container>
    </ng-container>

    <ng-template #templateChooser let-col="col">
        <ng-container *ngIf="col.children">
            <ng-container *ngTemplateOutlet="groupTemplate; context: { col: col }"></ng-container>
        </ng-container>

        <ng-container *ngIf="!col.children">
            <ng-container *ngTemplateOutlet="columnTemplate; context: { col: col }"></ng-container>
        </ng-container>
    </ng-template>

    <ng-template #groupTemplate let-col="col">
        <igx-column-group [header]="col.header">
            <ng-container *ngFor="let child of col.children">
                <ng-container *ngTemplateOutlet="templateChooser; context: { col: child }"></ng-container>
            </ng-container>
        </igx-column-group>
    </ng-template>

    <ng-template #columnTemplate let-col="col">
        <igx-column [field]="col.field" [header]="col.header"></igx-column>
    </ng-template>
</igx-grid>

And this almost works except that the column group never finds its children The main issue I think here is that the component IgxColumnGroupComponent uses @ContentChildren to find its children and in this case, its children exist in s separate named template and I am wondering if you changed descendants: true in the @ContentChildren declaration that It might be able to find them?

I have tride the following code and it works, but i would need to hard code in every level of depth I want

<igx-grid [data]="data">
    <ng-container *ngFor="let col of columns">
        <ng-container *ngIf="col && col.children">
            <ng-container *ngTemplateOutlet="groupTemplate; context: { col: col }"></ng-container>
        </ng-container>

        <ng-container *ngIf="col && !col.children">
            <igx-column [field]="col.field" [header]="col.header"></igx-column>
        </ng-container>
    </ng-container>

    <ng-template #groupTemplate let-col="col">
        <igx-column-group [header]="col.header">
            <ng-container *ngFor="let child1 of col.children">
                <igx-column *ngIf="!child1.children" [field]="child1.field" [header]="child1.header"></igx-column>
            </ng-container>
        </igx-column-group>
    </ng-template>
</igx-grid>

Describe alternatives you've considered

Another alternative would be if, as an alternative to setting column definitions via markup, if there was the ability to set them with a single input <igx-grid [columns]="myColumns">

kdinev commented 5 years ago

@rkaraivanov Will take a look at your suggestions and will comment on which one we will be implementing. If it's no secret, which grid were you using prior to the igx-grid?

rkaraivanov commented 5 years ago

@Lightw3ight

Unfortunately, even with descendants: true it won't work because of this issue, which may or may not be fixed by Ivy.

Basically the content children query can't be resolved for a tag/component which is not declared inside the parent content. Thus this thing will never find any column component as there is no column declaration in the context of the igx-column-group:

<ng-template #groupTemplate let-col="col">
    <igx-column-group [header]="col.header">
        <ng-container *ngFor="let child of col.children">
            <ng-container *ngTemplateOutlet="templateChooser; context: { col: child }"></ng-container>
        </ng-container>
    </igx-column-group>
</ng-template>

The grid does find the top level columns as they are defined in the context of the grid component itself.

I dig the idea for an input on the grid where you can pass a collection of column definitions observing a certain contract for the definition object

kdinev commented 5 years ago

@rkaraivanov I think that the grid input would be a rather simple solution, as it would do something similar to [autoGenerate]="true", but instead of looping through properties of a member of the data collection the grid is bound it, it would loop through objects of an IGridColumn type, which describes an IgxGridColumnComponent in terms for optional properties, which are the column inputs. The question is, how would it behave when both a [columns] input is provided, and IgxGridColumnComponents are defined in markup? The igGrid, for example, was handling autoGenerate + columns in a composite fashion, where columns were auto generated from the data collection member for each property that did not exist in the columns definition. What do you think?

rkaraivanov commented 5 years ago

@kdinev

  1. I'm not fond of composing between the passed collection and the defined columns in the markup. It can be a source of confusion for the users not to mention all the possible subtle bugs that can arise when trying to sync between the two. Users can still customize everything about the column and the underlying cells using the onColumnInit event and the column API.

  2. I suggest that the columns property takes precedence over the markup. It is an explicitly provided input for the grid the same way autoGenerate currently takes precedence over the markup.

@ChronosSF Care to share your thoughts on this?

ChronosSF commented 5 years ago

I agree with @rkaraivanov 's proposal based on our discussion earlier.

igGrid's behavior comes from the fact it doesn't expose another way (such as igx's onColumnInit event) to customize the auto-generation.

As the input would be complicated enough to implement on its own (requiring custom diff checker to allow users to manipulate the passed array instance, etc), I'd suggest against overcomplicating it by syncs with the markup collection.

kdinev commented 5 years ago

@ChronosSF Has any research been done on this so far?

ChronosSF commented 5 years ago

I don't think so. If you agree with the above suggestions we can implement a columns input with the suggested behavior of it taking precendence over autoGenerate and the markup.

skrustev commented 5 years ago

When implementing it I saw there is already a public getter called columns of type IgxColumnComponent[]. We will need to change the type of to IGridColumn[] in order to make it a setter as well and to accept column definitions. This leaves us with the question how to provide the array of IgxColumnComponents and there are a few options that can be considered:

@kdinev @ChronosSF @rkaraivanov Could you share your opinions what do you think would be better to have?

MayaKirova commented 5 years ago

@kdinev @ChronosSF @rkaraivanov

Currently since columns is just an Input that has no additional change detection in place (no custom diff checker) if you want to change something in the collection (add, remove, move a column or change a property of a column) you would have to set a new array for the property, for example: https://github.com/IgniteUI/igniteui-angular/pull/5922/files#diff-ca264b6473d9e1c38d63204de5b1bb37R131 When a new array is set, column instances are re-created so if the user has pinned, hidden or otherwise changed a column trough the UI that state will be lost.

Should we implement some additional handling so that changes are reflected automatically when the columns collection is changed by the user or should we leave it as a simple Input that requires the whole array to be changed for the change to be reflected in the grid?

ChronosSF commented 5 years ago

IMO, if we are to expose this input, it should be fully synced with the actual column instances collection i.e. having a column become pinned should update the object passed by the user and vice versa. Ideally, this would mean that each column has some sort of state property that holds all the props exposed in the interface and gets/sets to them directly. This also means creating columns from the template would create this state object that has the same footprint as the object expected by the columns input.

However, this will also be quite a cumbersome implementation when put against the issue it tries to solve. I would say that if we all agree we need full sync between columns and the column-like objects, we should probably drop it entirely.

skrustev commented 4 years ago

@kdinev @ChronosSF @rkaraivanov @MayaKirova

After discussing it was decided that this input should be dropped entirely due to the scope of it increasing too much when considering any syncing or detecting of the model that is passed being changed. For example changing a property of a column from pinned: true to pinned: false and detecting it without the need to recreate the columns.

The original issue can be solved by manually creating the grid columns and replacing them in the columnList query of the igxGrid component. Here's an example which supports any type of column currently available within the grid including multi column headers and multi row layout: https://stackblitz.com/edit/angular-hl5998

StefanRein commented 4 years ago

@Lightw3ight I am wondering, if you could make it work with ngProjectAs?