hypothesis / frontend-shared

UI components and styles for Hypothesis front-end applications
https://patterns.hypothes.is
5 stars 2 forks source link

[POC] Add new onClickRow callback to DataTable #1576

Closed acelaya closed 2 months ago

acelaya commented 2 months ago

This PR adds a new onClickRow callback to DataTable, that can be used to react to rows being clicked without taking selection into consideration.

Rationale

There's a "catch" on how this PR is implemented: Rows already handle an onClick callback to keep selected rows in sync. On paper, it shouldn't be a problem to handle both onSelectRow and onClickRow as part of the Row's onClick.

However, there's also a useArrowKeyNavigation in place which focusElement callback simulates a click event when focusing a row, so that the focused row is also selected when navigating with the arrow keys.

Unfortunately, this means the new onClickRow callback would also get invoked when focusing a row via arrow keys, which is not super intuitive.

In order to address that, in this PR I propose using a custom event to keep selected row in sync with focused one when using the arrow keys, to avoid the side effect described above.

The consequence is that it is now a bit less convenient to handle that event, as we now need to add and remove event handlers to all rows via side effects.

Alternatives

An alternative to this would be to still use a custom event, but use the name click so that we can capture both regular clicks and "custom" clicks via onClick.

Our "custom" click would include a different signature in the detail, so that we can tell one from the other.

<TableRow
  {...}
  onClick={e => {
    const isCustomClick = e instanceof CustomEvent;
    const shiftKey = isCustomClick ? e.detail.shiftKey : e.shiftKey;
    selectRow(row, shiftKey ? 'extend' : 'replace');
    if (!isCustomClick) {
      onClickRow?.(row);
    }
  }}
/>

This would reduce boilerplate code, but is a bit more hacky and less obvious.

A third alternative would be to make useArrowKeyNavigation's focusElement to infer the row object and call selectRow itself, instead of doing it as a side effect of a DOM event.

Testing

  1. Go to http://localhost:4001/data-datatable and search for "onClickRow".
  2. That prop includes an example where clicking a row will invoke console.log.
  3. Check that expected row model is logged when clicking on a row, but not when navigating rows via arrow keys.
acelaya commented 2 months ago

We may not need this in the end, so I'm going to close it for now.