gregnb / mui-datatables

Datatables for React using Material-UI
MIT License
2.7k stars 931 forks source link

customRender - index variable the row number? Can I get the column? #63

Closed teadams closed 6 years ago

teadams commented 6 years ago

I tested out custom Render and I believe the index passed to my function is the index of the row. Is there a way to get access to the column?

I'm trying to build a way to "in line edit" a cell. Use case a) user clicks on cell b) cell changes to input box c) when user leaves cell, update function is called

gregnb commented 6 years ago

I'll look into this might not be a bad idea to include the cell location

teadams commented 6 years ago

Please :) Right now I have a nasty hack where I sneak the column through the data. But other that that, it works awesome.

jkantr commented 6 years ago

I'll look into this might not be a bad idea to include the cell location

So I was going to create a new issue thread but I think I can continue it here.. the customRender function has a lot of short comings because in that context you have no access (that I can figure) to the rest of the table data.. or even just that own rows data! A minimal usecase:

Even if we had an 'isOwner' column or something.. I couldn't even check it given the current constraints (unless there is a part of the API I'm severely missing) -

One solution (the way an old library like datatables.net does it), is the render method of the columns definition provides (data, type, row, meta) where data is the cell data, row is the entire row data, and meta is the index data (row and column index) as well as the column "settings" (mui-datatables actually exposes column settings too.. you can get it from this in your customRender function context)

One way you could approach this is by exposing more data in the customRender fn, but I'm not sure what is the best way. I mean you could just keep adding params (like col index vs row index.. and row data) but i don't know if that is best?

Maybe best is just to make 1 breaking change to organize the best way to expose the most data.

Edit: just to make it clear also, I'm willing to implement the feature as well, once it's decided the best way .. i'm not only here to complain :)

teadams commented 6 years ago

Thanks... I was able to get the row information because my handleCustomRender has access to the data array. However, it would be far easier to directly have access to it in custom render.

This is the code

handleCustomRender(index, value, updateValue) {
    // index of column that contains the database key data
    const id_column_index = 0;

    /// THIS IS MY HACK TO WORK AROUND NOT KNOWING THE INDEX
    /// I pass the column in the fields of the data array
    var cell_data = value.split('//**//');
    var column = cell_data[0];  /// index of the column
    var value = cell_data[1];   /// original value before hack

    // in one case of using custom Render, it was using an index that was larger 
    // than size of the data.  This next line was to protect from that.
    if (this.state.data[index]) {
      // Get the data from the row that contains the ID
      id_column_value = this.state.data[index][id_column_index].split('//**//')[1];
    }

    return <Cell value={value}  column={column} id_column_value={id_column_value}  onUpdate={this.handleInlineUpdate}  />
}
gregnb commented 6 years ago

@teadams @jkantr I'm open to feedback on how either of you envision some changes toe customRender() in terms of structure. Let's see some quick mockups. How would we structure the additional params?

The current version is in beta so I am fine with making the right breaking change to give everyone lots of flexibility going forward.

Current:

handleCustomRender(index, value, updateValue) {

Becomes this:

handleCustomRender(updateValueFn, data) {

and data is an object where we could list as mentioned above a structure like the following:

data = {
  cellData,
  rowData,
  rowIndex,
  columnIndex,
}

I'm not crazy for naming the parameter data could use some help there. Thoughts? @jkantr I saw you had 'type' listed but what would that mean?

teadams commented 6 years ago

Maybe the Column Options for that column?

  1. This would support the existing options that you have defined, plus any extra options the developer added.
  2. This will serve as a way to pass any column specific options to the customRender function. (parallels column options for the full row)
teadams commented 6 years ago

I think data might be confused with the overall table data.

Some other ideas cell_vars cell_params cell_vals cell_attributes cell_props cell_state vars params vals attributes

jkantr commented 6 years ago

I guess this isn't a simple interface to design because the idea of a "custom" render is very general, but not very generalizable. Many cases it's just styling the data differently or only truncating it or wrapping it in some other tags or another component etc. But sometimes you're rending a custom column to have icons in it and no data, or an expandable button or something. Sometimes you want to have some sort of calculated data that's just a combination of data available elsewhere.

In theory all of this could be done when the data is being prepared to give to the component.. but there are advantages i think to having this behavior defined in the table (or column, in this case) as opposed to in the data transport interface. Especially in terms of rendering optimization.

One important thing to me is that the customRender function should be able to do its job without lexical/external access to the table data. I think the only way to keep the API clean is to make sure the fn is encapsulated with all the data it needs (as is, at the time it's called).

So option 1 is we put the most common use cases first, which is cellData (or cellValue?) and then we combine the rest of the data by its relation to the cell, so context (or related?) which would be things like the rowData, rowIndex and columnIndex... and then the meta which are things like the settings/state for the column filter, sort, sortDirection, display (or visibility?) and name. I believe this would cover most usecases I could think of... and at least by dividing them categorically we could expose something else later on without a breaking change.

I still have 1 other similar issue with accessing certain data from certain fields but I'll probably start another thread about it as i don't think it has anything to do directly with this fn's arguments.

gregnb commented 6 years ago

@jkantr Can you put together in one mock up function what you see for parameters and contents of any data objects

samsch commented 6 years ago

I'm doing some work with this library, and I need the row data available to my custom render. Specifically, my custom render uses an onEdit for a button which needs to be called with the row ID.

I think this would be a good shape: customRender(value, updateValueFn, meta), where meta is

{
  rowData,
  tableData,
  rowIndex,
  columnIndex,
  columnName,
  tableState: { /* filter, sort, selected, state etc. */ }
}

The tableState stuff isn't particularly important to what I'm doing personally, I added that in based on @jkantr last post.

jkantr commented 6 years ago

Having it all together in one value doesn't feel great... but it's almost arbitrary to try to split it up. I would probably opt for customRender(value, data: { rowData, tableData }, meta: { rowIndex, columnIndex, columnName, tableState }, updateValueFn) - probably only because having the 'callback' (or event trigger? w/e) function last seems more 'normal' to me.

But realistically I don't think any of this matters much. As long as the stuff is available and it's documented it's probably fine.

gregnb commented 6 years ago

I think I'm going to go with the following (and may actually start work on it today)

handleCustomRender(index, value, updateValueFn) 

and that will change to:

handleCustomRender(value, tableMeta, updateValueFn)
tableMeta {
 rowIndex,
 columnIndex,
 columnName,
 rowData,
 tableData,
 tableState: { /* filter, sort, selected, state etc */ }
}
gregnb commented 6 years ago

The solution outlined above has been merged in and published. Would like some feedback when you guys can provide it. @jkeruzec helped on the docs

jkeruzec commented 6 years ago

it seems that nothing is rendered with the change.. I return either a String or a component but column is empty/not displayed at all.

image

With :

{
                name: "Edit",
                options: {
                    filter: false,
                    customRender: (value, tableMetadata, updateValueFn) => {
                        return <IconButton><EditIcon /></IconButton>;
                        }
                }
            }
gregnb commented 6 years ago

@jkeruzec Yeah, it's weird when running locally as dev it works fine but I see some issues on prod bundles. let me look into this

gregnb commented 6 years ago

@jkeruzec Which version are you running? Make sure it's the latest beta.

Take a look at this codesandbox I put up with the same example that is in examples/ looks good here

https://codesandbox.io/s/w7zm4zm0r5

jkeruzec commented 6 years ago

Ok my bad, I was providing an array with on data less than the number of column... !

Thanks anyway !

ivaqmo123 commented 5 years ago

Hi Greg, When TableMeta.rowData is supposed to retrieve the full row data as an array. But as you can see in image below, rowData is only showing the value of the raw associated to the column (not the entire row as array). Could you please advise?

image

vishalc10 commented 5 years ago

@ivaqmo123 Having same issue, lemme know if you find a solution to it. Thanks

jlucier commented 5 years ago

I'm also seeing that tableMeta.tableData is an empty list.

gabrielliwerant commented 5 years ago

Opened a new issue to address some of the issues with tableMeta https://github.com/gregnb/mui-datatables/issues/901.