feat: allow override checkbox in Controlled tables

[Thomas-Boi]

Fixes https://github.com/imballinst/react-bs-datatable/issues/173. This PR should reset the checkbox selected state to an empty Set(). This allows deselection across pages. See #173 for more details on the problem.

[imballinst]

@Thomas-Boi I pushed some commits into your branch and have published it as 3.10.0-beta.0 with sandbox link here using your example: https://codesandbox.io/s/naughty-mopsa-pvpus5?file=/src/App.js.

Please let me know if there are holes in the experience and I'll try to patch it tomorrow morning. Thanks!

[Thomas-Boi]

Thanks @imballinst! I installed the beta package and tested it with my project. It runs just like I wanted 👍 so thank you very much 😄

There is one small bug involving the checkbox in the header though. If you select a few rows spread across a few pages, the header checkbox will eventually go out of sync with the rows being displayed. You can replicate this in your sandbox as well:

• Select a few rows (selected 2)

• Go to next page. The rows are empty but the header checkbox has a - sign that only appears when some rows are selected.

• Click on header checkbox to select all on the current page:

• Go back to first page, all the checkboxes look fine.

• Click header checkbox to select all on the page. Checkbox now has the - icon which is strange. Usually, selecting all would show the ☑️ icon.

• Click header checkbox again to deselect all on the page. Checkbox now has the ☑️ icon which is strange since that icon only shows when all the rows on the page are selected:

• Clear Selection thankfully reset everything normally:

Normally, if I click on header checkbox to select all rows, the header checkbox will look like this (☑️ icon):

In summary: the header checkbox is not reflecting the state of the selected rows of the current page.

Suggested solution: perhaps change the header checkbox state by checking whether the rows being shown are in the Set()? This way, the header checkbox is always limited to the current row only. I don't know whether this will be computationally expensive though.

[imballinst]

@Thomas-Boi hmmmm, I see. I think it's because of this line:

https://github.com/imballinst/react-bs-datatable/blob/af2f336a2e34a8746a71f6c60377bf5b545a2009/src/helpers/checkbox.ts#L94-L100

I tried with this scenario:

1. Select 2 rows on the first page
2. Select 1 row on the second page
3. Since each page contains only 3 rows and currently number of selected row is 3, then 3 === 3, hence the table header checkbox becomes a ✅
4. If I select another row, then number of selected row is 4, then 4 !== 3, hence the table header checkbox becomes indeterminate.

I'm actually thinking on swapping these lines:

https://github.com/imballinst/react-bs-datatable/blob/af2f336a2e34a8746a71f6c60377bf5b545a2009/src/helpers/hooks.ts#L157-L162

That way, we don't need the callback function for the handler like in this PR. For example, I can just pass this to the DatatableWrapper:

<DatatableWrapper
  headers={headers}
  body={pages[pageIdx]}
  checkboxProps={{
    onCheckboxChange: (result) => {
      result.nextCheckboxState = {
        selected: new Set(),
        state: "all-selected"
      };
    }
  }}
>

What do you think about it? That way, all checkbox changes for uncontrolled tables are centralized in the checkboxProps.onCheckboxChange. There, we can also "compute" for controlled-but-uncontrolled table states, for example, if we have data on the data count on the backend, then we can "override" the state there ourselves.

nextCheckboxState.state = nextCheckboxState.selected.size === 0 ? 'none-selected' : nextCheckboxState.selected.size === totalDataInBackend ? 'all-selected' : 'some-selected'

[Thomas-Boi]

Regarding this chunk of code:

<DatatableWrapper
  headers={headers}
  body={pages[pageIdx]}
  checkboxProps={{
    onCheckboxChange: (result) => {
      result.nextCheckboxState = {
        selected: new Set(),
        state: "all-selected"
      };
    }
  }}
>

I think this could work as long as I can still use the default functionality after I override some parts of the nextCheckboxState.

There, we can also "compute" for controlled-but-uncontrolled table states, for example, if we have data on the data count on the backend, then we can "override" the state there ourselves. nextCheckboxState.state = nextCheckboxState.selected.size === 0 ? 'none-selected' : nextCheckboxState.selected.size === totalDataInBackend ? 'all-selected' : 'some-selected'

This could work as well. For my case, I would like the header checkbox to be per page (e.g. page 1 all selected => header should be ☑️ , page 2 empty => header should be empty). In that case, I'd have to manually check the data I'm displaying against nextCheckbox.selected and see whether it's all-selected, some-selected or none-selected, right?

Something like:

let count = getCountOfRowInSelected(body, nextCheckboxState.selected)
nextCheckboxState.state  = count === 0 ? 'none-selected' :  nextCheckboxState.selected.size ===  count ? "all-selected" : 'none-selected`

If I can do the above lines of code, I think your approach should work great for me 👍

[imballinst]

In that case, I'd have to manually check the data I'm displaying against nextCheckbox.selected and see whether it's all-selected, some-selected or none-selected, right?

Exactly, yes! Alright, let me try working on it tonight my time and I'll see if I can publish a beta for you to try.

[imballinst]

@Thomas-Boi after experimenting a bit, I think this is very complex to do in uncontrolled table, since we may have filter, pagination, and sort which all may change the rows in a page. I'm not sure if it's worth it if we specify it that deep.

Would it be possible with the current implementation if you use a controlled table instead? So you could control all the filter/sort/pagination/checkbox states and no need to rely on the internals of the table.

[Thomas-Boi]

@imballinst that's fine with me as well! I just need a way to hook into the table's functionality without overriding everything if that makes sense. I remember trying out the controlled checkbox state but couldn't get the onCheckboxChange callback working. Is that functioning now? Can I still use the underlying default functionality (updating the icons etc.) even when I "control" certain parts?

[imballinst]

@Thomas-Boi hmmmm, that's a good point. Let me check in a sandbox and I try it myself first.

[imballinst]

Here's my experiment so far: https://codesandbox.io/s/dark-snowflake-9srvdz?file=/src/App.tsx. Currently I think it's blocked by these lines: https://github.com/imballinst/react-bs-datatable/blob/main/src/helpers/checkbox.ts#L50-L58. So on top of handling controlled table event for select/unselect all, I also need to handle the selection one-by-one.

I'll try to update the package for that part and I'll update my sandbox after. Probably around tonight my time as usual, though.

[Thomas-Boi]

No problem @imballinst ! Thank you for spending time on this, I really appreciate it.

[imballinst]

hi @Thomas-Boi, could you help verify my controlled table example here? Thanks! https://codesandbox.io/s/dark-snowflake-9srvdz?file=/src/App.tsx

The important parts are (so we first override the indeterminate behavior, then second, every time we "fetch" or change page, we "redefine" the checkbox states so sort/filter/paginate will result in consistent behavior):

const onCheckboxChange: CheckboxOnChange = (result, _event) => {
  // Manually modifying these below will cause unexpected behavior
  // when we select a checkbox one-by-one in controlled mode.
  if (
    data.every((item) => result.nextCheckboxState.selected.has(item.name))
  ) {
    result.nextCheckboxState.state = "all-selected";
  } else {
    result.nextCheckboxState.state = "none-selected";
  }

  setCheckboxState((oldState) => ({
    ...oldState,
    [result.checkboxProp]: result.nextCheckboxState
  }));
};

and this:

async function getData() {
  const response = await fetchControlledMockData({
    filter,
    sortState,
    currentPage,
    rowsPerPage
  });

  setCheckboxState((oldState) => ({
    checkbox: {
      selected: oldState.checkbox.selected,
      state: response.data.every((item) =>
        oldState.checkbox.selected.has(item.name)
      )
        ? "all-selected"
        : "none-selected"
    }
  }));

  // ...
}

[Thomas-Boi]

Hi @imballinst ,

Your code makes sense and I tested it out. Unfortunately, I couldn't get the checkbox in the <TableBody> to work. The one in the <TableHeader> works great and you can verify this with the console.log in this sandbox. Once both the body and the header modifies the same checkbox state, I think I'll be good to go.

[imballinst]

hi @Thomas-Boi, could you try it out in this sandbox? https://codesandbox.io/s/epic-herschel-uc6285?file=/src/App.js

I made few adjustments:

// Pass controlled parameters to useCreateCheckboxHandlers, so that it'll be using our data and handlers instead of the built-in ones (for uncontrolled)
const { createBulkCheckboxClickHandler } = useCreateCheckboxHandlers({
  checkboxState,
  data,
  filteredDataLength: pages.flat().length,
  onCheckboxChange
});

// Lift the checkbox state up so that it's in the same level with the pagination state.
// Then, on page change, check if all rows in the page exist in the selected (to determine all selected or none selected)
const nextBody = pages[targetPage];

setBody(nextBody);
setCheckboxState((oldState) => ({
  checkbox: {
    selected: oldState.checkbox.selected,
    state: nextBody.every((item) =>
      oldState.checkbox.selected.has(item.id)
    )
      ? "all-selected"
      : "none-selected"
  }

Also I think there was a typo in the id prop that should be used: it was name instead of id. Let me know if there are things that are still incompatible. Thanks!

[Thomas-Boi]

Hi @imballinst ,

Thank you so much for your help! Your beta version works with my project and it does exactly what I wanted. I definitely forgot that using <TableRow> will override the props in <TableBody>.

Thank you for your time and this looks and work great 😄

[imballinst]

Sounds good! I'll merge this PR, then. Thanks for your confirmation @Thomas-Boi!