Feature Request: Update the `rowOnClick` logic so inner children can trigger it

[Thomas-Boi]

Hello there 👋 ,

Background I recently spotted this bug trying to customize a row. I have a row that needs to use some <p> tags for styling, thus I manually modified the data before passing it to the DatatableWrapper's body prop.

The reason why I didn't make my own <TableRow> is because I needed to use the controlledProps's checkboxes props. From what I see in the code, this is not easy to write so I used the built in TableRow instead.

Problem

When the user clicks on the <p> tags, the rowOnClick is not triggered. After checking the code, I narrow it down to this:

As you can see, if an element is not a <td> or a <tr>, the callback is not triggered.

Request

I have a feeling that I was the reason for this change. However, due to changing requirements, would you please remove this code 😅? If a developer doesn't want the event to propagate, I think it'll be better to let them call event.stopPropagation manually on their end for maximum flexibility.

[imballinst]

hey @Thomas-Boi!

I have a feeling that I was the reason for this change. However, due to changing requirements, would you please remove this code 😅

I guess you did? 😆

With regards to that, unfortunately the event.stopPropagation is only able to stop bubbling if the child event is fired first. If it's the parent event that's fired first, then I guess it's not possible. Example, using the old datatable 3.7.0 where the TD, TR guard was not present: https://codesandbox.io/s/priceless-wave-5whs93?file=/src/App.tsx

Notice that when we click the checkbox, the row click is fired first before the checkbox changes.

While dropping it is tempting, but it means breaking existing behavior (which I kinda don't want to, unless I'm about to bump the major version). Instead, what do you think on allowing it to be passed via props? So:

interface TableRowProps {
  validRowClickElements?: string | string[]
}

<TableRow /> // defaults to ['TR', 'TD']
<TableRow validRowClickElements="*" /> // row on click are triggered by all elements inside the row/column, including the row/column itself
<TableRow validRowClickElements={['TD']} /> // row on click only triggered by column clicks

[Thomas-Boi]

<TableRow /> // defaults to ['TR', 'TD']
<TableRow validRowClickElements="*" /> // row on click are triggered by all elements inside the row/column, including the row/column itself
<TableRow validRowClickElements={['TD']} /> // row on click only triggered by column clicks

The above solution sounds great 😄 . Thank you for digging up the reason for the change, I didn't remember the PR where I brought it up.

[imballinst]

hi @Thomas-Boi, I have published 3.12.0-beta.0 and here's the sandbox: https://codesandbox.io/s/unruffled-black-36vc6h?file=/src/App.ts. Let me know what you think, if everything is OK then I can publish the stable version tomorrow morning my time.

Thanks!

[Thomas-Boi]

Thank you for your work! The <p> tag works as well. Appreciate you getting it done so quickly.

[imballinst]

Sounds good! I've published the stable version https://www.npmjs.com/package/react-bs-datatable/v/3.12.0.

Let me know if you run into any kind of issues @Thomas-Boi. Thanks!