How to add checkbox and select all?

[urvish91]

Thanks for making such nice library. it looks awesome to me. but I stuck that I need first column with checkbox and select all from header. so can you please help me with that?

[urvish91]

Fixed

[KapitanOczywisty]

@urvish91 I'm curious how did you do this? Anyway this should be a standard feature, so you may leave this open.

[imballinst]

Hey folks, sorry I was quite busy past 2 weeks. If I may guess, @urvish91 is using headerCell and cell for the checkboxes. Example can be seen here https://codesandbox.io/s/laughing-sky-lqy7o.

I agree that this feature can be useful and it is less hassle for this library's users. I am reopening this and proposing an API.

const headers = [
  {
    title: " ",
    prop: "checkbox"
    // New API: pass 2 props of checkbox handlers.
    checkbox: {
      onCheckRow: (rowData) => { 
        // Mutate state here...
      },
      onCheckAllRows: () => { 
        // Mutate state here...
      }
    }
  }
]

Hence, if checkbox field is passed, that column will automatically become a checkbox column, provided that onCheckRow and onCheckAllRows are also provided.

The alternative API that I'm proposing is this:

<Datatable
  // Other props...
  // New API: pass 2 props of checkbox handlers.
  onChangeRowCheckbox={(rowData) => handleChangeRowCheckbox(rowData)}
  // Of course it can be `onChangeRowCheckbox={handleChangeRowCheckbox}` as well.
  onChangeAllRowsCheckbox={handleChangeAllRowsCheckbox}
/>

Please let me know your thoughts @urvish91 @KapitanOczywisty regarding which one might be better. Thanks!

[KapitanOczywisty]

@Imballinst It probably would make sense to select all on current page, not on all (but remember checked), and I wonder how this would work with remote pagination. Somebody could have alert above table like N items selected on all pages. [clear selection], API should have ways to make this possible.

Also It would be nice if onRowClick would toggle selection. I did my implementation with refs, but I have a hard time to deal properly with onRowClick, since it has no event passed, and you cannot check event.target to exclude click on checkbox itself (nor on action buttons).

[imballinst]

It probably would make sense to select all on current page, not on all (but remember checked), and I wonder how this would work with remote pagination. Somebody could have alert above table like N items selected on all pages. [clear selection], API should have ways to make this possible.

I agree on to select all on current page.

With regards to "Clear selection", perhaps we can also add "Select all N items" to the left of it (in case we want to select all items in entire table). However, I'm not sure if we should provide this feature. Maybe we can default it to be enabled then provide a tableOptions prop to disable it?

Also It would be nice if onRowClick would toggle selection. I did my implementation with refs, but I have a hard time to deal properly with onRowClick, since it has no event passed, and you cannot check event.target to exclude click on checkbox itself (nor on action buttons).

Does the current version not support it? I think it can be done (I updated the previous example to use onRowClick with checkboxes): https://codesandbox.io/s/laughing-sky-lqy7o?file=/src/index.js.

[KapitanOczywisty]

I've misunderstood proposition a bit, I thought that state will be managed by library, but both points are no more actual with user-managed state.

With regards to "Clear selection", perhaps we can also add "Select all N items" to the left of it (in case we want to select all items in entire table). However, I'm not sure if we should provide this feature. Maybe we can default it to be enabled then provide a tableOptions prop to disable it?

It should be only possible for user to implement, but if state is managed by user it's possible regardless. https://codesandbox.io/s/211-checkboxes-forked-txlkv?file=/src/index.js

Does the current version not support it? I think it can be done (I updated the previous example to use onRowClick with checkboxes): https://codesandbox.io/s/laughing-sky-lqy7o?file=/src/index.js.

I meant this situation in particular: https://codesandbox.io/s/211-checkboxes-forked-e8271?file=/src/index.js:1743-1814 , but I've fixed it with <ButtonGroup onClick={(event) => event.stopPropagation()}>

[imballinst]

Ah, okay! Yes, I think that's a quite common situation when an interactive HTML element is a children of a non-interactive element+event handler. Nicely done by using .stopPropagation(). That being said, I think I will make sure to do the propagation underneath the table's implementation so users don't have to deal with it.

Thanks for the discussion! Anything else that I should add into the detail of the implementation?

[KapitanOczywisty]

So if user is managing state: In what format selections will be passed to <Datatable>?

From what I can see in other tables:

1. would be nice to allow setting indeterminate to select all checkbox. Not elegant for current version: https://codesandbox.io/s/datatable-checkboxes-indeterminate-6c7m0
2. option to not show select all and only row-level selection
3. prevent selection on particular rows
4. hide checkbox and highlight rows instead; somewhat-possible now: https://codesandbox.io/s/211-datatable-checkboxes-highlight-ohkef , we need row-level className callback

[imballinst]

So if user is managing state: In what format selections will be passed to <Datatable>?

It's a little bit tricky. As of now, I can only think of 2 solutions (I'm more leaning to the first one):

1. The table automatically "computes" the unique ID: we don't have to do anything different compared to what we have at the moment. This may be a bit tricky when we allow dynamically addition/deletion of the table data on-the-fly. The easiest workaround would be: "When tableBody changes, reset the checked rows state". But I'll see what I can do.
2. The data passed to tableBody to have something like __id field for identifier

const tableBody = [{ __id: 0, ... }, { __id: 1, ... }];
// ...
return <Datatable tableBody={tableBody} />

The function handler for both approaches above will be:

function onChangeCheckedRows(data) {
  console.log(data);
  // [{ __id: 0, rowData: { ... } }, { __id: 1, rowData: { ... } }, { __id: 2, rowData: { ... } }]
}

That way, it will be flexible:

1. If we want to read the checked row data, we just see the rowData.
2. if we want to control the state, then we use the __id fields and probably pass the array to a prop like checkedRowIds which is an array of __id field values.

---

About your other questions:

1. would be nice to allow setting indeterminate to select all checkbox. Not elegant for current version:
2. option to not show select all and only row-level selection

TIL about indeterminate. I agree with your concern. I am thinking of adding a new API as the following:

const tableHeaders = [{
  checkbox: {
    // `checkbox` field consists of 2 optional fields: `header` and `body`.
    // Each of them have the same data structure:
    // `{ indeterminate: callbackFn, checked: callbackFn, hidden: boolean }`
    // Callback function: (checkedRows, tableProps) => boolean.
    // The first parameter of the callback function is the "internal" table checked rows state.
    header: {
      // Uncontrolled.
      indeterminate: (checkedRows, tableProps) => checkedRows.length > 0 && checkedRows.length < tableProps.tableBody.length
      // Controlled.
      indeterminate: (_checkedRows, tableProps) => tableProps.checkedRows.length > 0 && tableProps.checkedRows.length < tableProps.tableBody.length,
      checked: (_checkedRows, tableProps) => tableProps.checkedRows.length === tableProps.tableBody.length,
      hidden: true // if we want to hide it. Defaults to `undefined`, which equals to `false`. Or pass a function, for example, `(data) => data.score < 50`.
    },
    body: {
      // Same fields like what we have in `checkbox.header`.
    }
  }
}]

The reason why I pass tableProps to the callback function is because I don't want to create tableHeaders over and over again in the main component after each state change (not that much of performance downgrade, but still, I prefer passing parameters instead).

3. prevent selection on particular rows

Hmmm... onBeforeChangeCheckedRows(data) which returns boolean, I guess? So internally in the table, it would be something like this:

function onCheckRow(data) {
  // When the condition is not met, prevent calling the on change checked rows.
  if (!props.onBeforeChangeCheckedRows(data)) {
    return;
  }

  if (props.onChangeCheckedRows) {
    // Controlled.
    props.onChangeCheckedRows(data);
  } else {
    // Uncontrolled.
    onChangeCheckedRows(data);
  }
}

4. hide checkbox and highlight rows instead; somewhat-possible now: https://codesandbox.io/s/211-datatable-checkboxes-highlight-ohkef , we need row-level className callback

We can remove the cell custom-rendering in tableHeaders, right? https://codesandbox.io/s/datatable-checkboxes-highlight-without-checkbox-kjsr5.

Apologies for lengthy comment -- I want to make sure I provide the clearest intention. In any case, if there are parts of my explanation that aren't clear to you, please let me know. Thanks!

[KapitanOczywisty]

Passing selection

For this to make sense, you need unique key in data anyway, so I'm proposing to use column.

const tableHeaders = [{
  prop: "domain_id",
  checkbox: {
    onSelect...
    onSelectAll...
}
}];
return <Datatable selected={selected} ...>

It is relying on user to keep state, but it's way more powerful to use. And stuff like remote pagination should work out-of-the-box.

Indeterminate

Maybe better to keep one function to manage states:

const tableHeaders = [{
  checkbox: {
// by default
    // "all" | "some" | "none"
    selectAllState: (selected, tableProps) => selected.length === 0 ? "none" : (selected.length === tableProps.tableBody.length ? "all" : "some"),
    selected: (row) => selected.includes(row.id), // row[prop] by default
    selectOnRowClick: true,
// required from user
    onSelectAll: (selectAllState) => selectAllState === "none" ? doSelectAll() : doSelectNone(),
    onSelect: (row)=>toggleSelect(row),
  }
}];

Note: There is no need for indeterminate for rows.

Hidden SelectAll / Column

We can deal with that using styles, but we're missing class name for header cell th, this would be beneficial also for responsiveness (e.g. hide column on breakpoint className: "d-none d-md-table-cell").

const tableHeaders = [{
  // checkbox stuff
  headerProps: {
    className: "d-none"
  },
  cellProps: {
    className: "d-none"
  }
}];

Hacked working version https://codesandbox.io/s/datatable-checkboxes-highlight-hidden-qcesi

Prevent selection

By having control over onSelect, there is no problem with that. We need to manage style anyway to indicate nonselectability. Maybe we could use something like rowStyle to add class: tr.row-unselectable.

Edit:

Selection format

selection beside array should allow Set, which is way better optimized for keeping track of the selections. For simplicity I'd even support only Set by default. For other formats user would be forced to rewrite onSelect etc.

Quick start

Datatable could probably implement also selection state internally, to have only prop: "id", checkbox:true required to get this working. I'm just not entirely sure how user would extract selections cleanly (I'm starting with react), without ref hackery.

However, even this would be nice for basic usage

const selected = new Set();
const tableHeaders = [{
  prop: "user_id",
  checkbox: true
},
// ...
];
return <Datatable selected={selected} tableHeaders={tableHeaders} tableBody={tableBody}>

[imballinst]

After taking some time to digest your suggestion, I think those are good APIs, in particular the selectAllState. It's neat and covers all 3 conditions in 1 state without us having to "compute" them in other functions.

I only have 2 comments:

1. Set and/or Array: I think I'd support both but recommend explicitly to use Set in the documentation. I am not sure yet if all users are familiar of Set. Maybe in future versions after this we'll start deprecating the support of Array in selected prop.
2. Quick start: One way that we could do it is that to pass a prop onSelectionChange (to the table) such as:

function onSelectionChange(selected: Set<string>, column: HeaderType) {
  // ...do stuff.
}
// ...
return <Datatable onSelectionChange={onSelectionChange} />

But then, we will have 2 ways to utilize column-based checkbox and I don't know if that's confusing or not. As of now I'm more leaning towards supporting controlled state only.

[KapitanOczywisty]

I am not sure yet if all users are familiar of Set.

If you're using react you're probably familiar with ES6. However it's not that hard to write support for both, so whatever is good.

Set<string>

I think Set<any> since data could be of any type.

But then, we will have 2 ways to utilize column-based checkbox

Maybe selection prop should be of type Set<any> | (Set<any>) => void, where function would be practically onSelectionChange when state is managed internally.

I think those are good APIs

Thanks, I've researched a bit other datatable scripts to get feeling what could be needed. Though, There are still ways to make it prettier ;)

---

I wonder if there is need to have more than one checkbox column? It could be a bit more cleaner if checkbox settings would be moved to <Datatable ... />

const selected = new Set();
const tableHeaders = [{
  prop: "user_id",
  checkbox: true
},
// ...
];
const checkboxProps = {
  onSelectAll: (selectAllState) => selectAllState === "none" ? doSelectAll() : doSelectNone(),
  onSelect: (row)=>toggleSelect(row),
  selectOnRowClick: true,
  selected,
};
return <Datatable checkboxProps={checkboxProps} tableHeaders={tableHeaders} tableBody={tableBody}>

[imballinst]

Maybe selection prop should be of type Set | (Set) => void, where function would be practically onSelectionChange when state is managed internally.

Hmm, I'm not entirely sure about this. I like onSelectionChange (an explicit, different prop) better than selection. Mostly semantic, though—because selection indicates an object, whereas onSelectionChange indicates a function.

I wonder if there is need to have more than one checkbox column? It could be a bit more cleaner if checkbox settings would be moved to <Datatable ... />

It's not likely for a table to have more than 1 checkbox, but I'm considering the difficulty to change the checkbox placement. If we control the checkbox in the tableHeaders, then the checkbox column number is linear with its index in tableHeaders.

If we pass it directly to <Datatable>, that'd be quite awkward, isn't it? It is because now the table headers content and order are affected by 2 variables, which possibly located quite far from each other in the file (code-wise).

[KapitanOczywisty]

I wonder if there is need to have more than one checkbox column? It could be a bit more cleaner if checkbox settings would be moved to <Datatable ... />

It's not likely for a table to have more than 1 checkbox, but I'm considering the difficulty to change the checkbox placement. If we control the checkbox in the tableHeaders, then the checkbox column number is linear with its index in tableHeaders.

You don't really need to know column number, you're propagating props to the headers and rows, these with checkbox:true will deal with implementation. Only maybe set defaults somewhere higher.

If we pass it directly to <Datatable>, that'd be quite awkward, isn't it? It is because now the table headers content and order are affected by 2 variables, which possibly located quite far from each other in the file (code-wise).

I'm not sure what you mean. I'm concerned about growing tableHeaders, It'll be hard to see table structure past all these options.

[imballinst]

You don't really need to know column number, you're propagating props to the headers and rows, these with checkbox:true will deal with implementation. Only maybe set defaults somewhere higher.

Aha! Apologies, I misunderstood your intention. I think this clears it.

In that case, I agree about moving the checkbox options to the <Datatable> instead. So that way, all checkbox columns (if we have more than 1) will have the same "behavior", without having to declare the same thing over and over again—is that what you meant (?)

[KapitanOczywisty]

Yes 😃 Mention If there will be anything for beta testing 😉

[imballinst]

OK! Thanks for the discussion. I'll see if I can get that done this week 😄