Group Render Strategy consistency

[vitolpoc]

Playing with the multiple groupRenderStrategy, the way the "inline" strategy works (and the documentation) is a bit puzzling.
Indeed I was expecting that the "inline" strategy works with the same inputs as the other strategies by default.

Here is the scenario (based on https://infinite-table.com/docs/latest/learn/grouping-and-pivoting/grouping-rows ):

• created a grid with "multiple-columns" strategy + set the groupRowsBy with the column property + remove this column from the list of the columns map definition to not have it duplicated in the table
• then switch to "single" strategy: works as expected
• then switch to "inline" strategy: does not work as I thought, the group was not displayed.

Indeed I figured out that for this particular mode we need to put back the definition of the group column inside the columns map definition. It's a bit inconsistent. What do you think ?

Here is a code sandbox that shows the "issue": https://codesandbox.io/s/sandpack-project-forked-5jg1y?file=/src/App.tsx

[roblotter]

The way we thought this is that inline does not generate any new column but uses the current columns. The docs mention this

no group columns are generated

but we clearly failed to document it properly and explain it thoroughly. To be honest, at runtime people are not very likely to switch the group rendering strategy.
Probably we need a console warning when you specify inline group rendering, if you also explicitly pass a column in DataSource.groupRowsBy or pass a groupColumn in InfiniteTable props.

If you think of a better way to specify this or a better API, let us know as we're keen to consider any improvements we can make.

[vitolpoc]

In fact, the most surprising at first is that: "inline" is another value of the group renderer strategy which let think it's controlling only the way it is displayed and not the other configuration inputs (i.e. groupRowsBy & columns).
In other terms I would expect that by changing (or simply setting) the value to "inline" I would not have to change the other configuration inputs (how the datasource is configured, what are the columns displayed) because from a user perspective it's just a display option

[roblotter]

Another tip: we're using https://npmjs.com/package/debug, so you can do localStorage.debug = '*' to see lots of internal logging messages, or localStorage.debug = 'InfiniteTable:rerender', namely localStorage.debug = 'channel1,channel2, etc' if you ever have issues and need to send a log of what's happening.

[roblotter]

We have been thinking extensively about this scenario, so trying to document our thinking in the lines below:

We're on the same page as you that it should be easier to switch from single/multi column group render strategy to inline, without having to manually hide/show the column you group by. We should have an easier way of doing this.
So we were thinking to implement a prop called something like hideColumnsWhenGroupedBy - it would default to true and it would hide a column whenever the column.field and groupRowsBy.field match. So whenever the datasource is grouped by a field that a column is mapped to, that column would be hidden.
Think this would be a useful addition, albeit a bit specific.

Then we tried to take another, more general approach - namely manage column visibility in a more powerful way.
We are considering offering a prop columnVisibility (and it's corresponding defaultColumnVisibility that could receive the following values:

• an object - keys being column ids while values being true, false (maybe also allow "hiddenWhenGrouped"? - still thinking about this one). Columns not explicitly mentioned in this object would be visible by default.
• a function - this would only be applicable for controlled columnVisibility prop. The function could have the following signature:

columnVisibility=({ column, viewportSize, groupRowsBy, pivotBy, sortInfo, ... etc } => {
    
   if (viewportSize.width < 500) {
      return column.id in COLS_FOR_MOBILE
   }

   if (column.groupByField){ 
      return true
   } 

   if (groupRowsBy){
      return !groupRowsBy.map(g=>g.field).includes(c.field)
   }
})

const COLS_FOR_MOBILE = { name: true, id: true, age: true }

The signature is not set in stone yet - still thinking about this.
The idea is that accepting a function would be highly dynamic and very powerful - however not something that cannot be done with just an object because you could hook to onSortInfoChange, onViewportSizeChange (coming), onGroupRowsByChange (also coming) and re-render the table with a new value for the columnVisibility object - so basically reconstruct it any time you are interested in changing the column visibility.
So accepting a function for column visibility would give power users a syntactic sugar, however, it's not immediately clear (without docs) when would this function be called - eg: the table is resized, the grouping is changed, etc

We're also considering how this would integrate with the column menu (also coming) that would allow end-users to select visible columns. The user selecting columns to show/hide would obviously call onColumnVisibilityChange, but if we will support columnVisibility as a function, it will need to receive as an argument the user selection. This is definitely possible, but maybe a bit cumbersome.

So we're not entirely sold on the idea of columnVisibility as a function - rather, as a first step we would implement columnVisibility as an object, with true, false and "hiddenWhenGrouped". However, when the end-user would select from the column menu some columns to show/hide, it would trigger onColumnVisibilityChange, with the new object that describes the visibility. The end-user has no way to know when a column is configured with hiddenWhenGrouped, and clicking the checkbox to show it would simply give us an object with true for the key of that column id. So probably it's better to move hiddenWhenGrouped as a property on the column object.

TLDR

We want to make this case easier to manage, so we want to implement the following:

• columnVisibility/defaultColumnVisibility - an object with keys being object ids and values being boolean true/false. Columns not mentioned in this object will default to true - being visible.
• onColumnVisibilityChange(columnVisibility) - will be called whenever the visibility will change via the (coming) column menu
• columns will receive a hiddenWhenGrouped property
• we're almost done implementing column types (in order to address Custom type + custom cell renderer + custom header cell renderer #10 ), so the hiddenWhenGrouped property will also be available for column types - and thus get transferred to columns.
• in the near future, we'll implement onViewportSizeChange(size) so if you're interested you can use controlled columnVisibility to hide/show columns based on the available size (width) - useful if you want to customize the table and column visibility for smaller screen sizes.

Let us know what you think about the proposals.

[vitolpoc]

Thanks for the time spent on it.

With the addition of columnVisibility, it means the whole columns configuration (groupRowsBy, columns, hideEmptyGroupColumns, ...) can remain the same, no matter what is the group render strategy value.

In other word (and correct me if I'm wrong), as a grid user, we'll need to define:

• the full list of column definitions in the columns properties (without taking into account their visibility)
• the list of grouped columns in groupRowsBy, mainly the keys (+ pivot + aggregation + ...)
• set the new columnVisibility list to control their display or not
  then when we can change the group display without changing anything else.

This change will make then the configuration consistent.

[roblotter]

Yes, that's right. We're working on shipping column types and column visibility right now. Also better server-side support in the works, as discussed