Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data views: Add click-to-select behavior on table rows #59803

Merged
merged 11 commits into from
Mar 22, 2024
+36 −13
Merged

Data views: Add click-to-select behavior on table rows #59803

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b9064d4
Clicking the checkbox container toggles selection
jameskoster Mar 12, 2024
4880f5d
Fix checkboxes
jameskoster Mar 12, 2024
146418f
Click row to select
jameskoster Mar 13, 2024
e3766a2
Style adjustments
jameskoster Mar 13, 2024
6db4bb9
Merge branch 'trunk' into update/data-views-checkbox-hitbox
jameskoster Mar 15, 2024
e2bf36c
Demo: allow click-on-row with meta/cmd or when selection already active
mcsf Mar 15, 2024
242966d
Demo: always allow click-on-row, taking care not to catch button clicks
mcsf Mar 15, 2024
3541f13
Bonus: disable item actions if a selection is already active
mcsf Mar 15, 2024
fa91886
Revert "Bonus: disable item actions if a selection is already active"
jameskoster Mar 18, 2024
64c4967
Do no select record when selecting text
jameskoster Mar 18, 2024
be1856a
Disable click-to-select on touch devices (use onTouchStart)
mcsf Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/dataviews/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,12 @@
> * {
flex-grow: 1;
}

&.dataviews-view-table__primary-field {
a {
flex-grow: 0;
}
}
}
}
.dataviews-view-table-header-button {
Expand Down Expand Up @@ -236,6 +242,7 @@
white-space: nowrap;
display: block;
width: 100%;
overflow: hidden;

a {
text-decoration: none;
Expand All @@ -244,10 +251,10 @@
white-space: nowrap;
overflow: hidden;
display: block;
width: 100%;
flex-grow: 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this happens as a result of this change, but here's what I see for the focus ring:
Captura de ecrã 2024-03-14, às 12 06 26

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskoster it sounds like this PR introduced this regression:

Before After
Captura de ecrã 2024-03-25, às 16 38 55 Captura de ecrã 2024-03-25, às 16 38 44

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh shoot, apologies I forgot to address this. I'll make a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#60191


&:hover {
color: $gray-900;
color: var(--wp-admin-theme-color);
}
@include link-reset();
}
Expand Down
27 changes: 16 additions & 11 deletions packages/dataviews/src/view-table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,6 @@ function TableRow( {
data,
} ) {
const hasPossibleBulkAction = useHasAPossibleBulkAction( actions, item );

const isSelected = selection.includes( id );

const [ isHovered, setIsHovered ] = useState( false );
Expand All @@ -253,19 +252,14 @@ function TableRow( {
return (
<tr
className={ classnames( 'dataviews-view-table__row', {
'is-selected':
hasPossibleBulkAction && selection.includes( id ),
'is-selected': hasPossibleBulkAction && isSelected,
'is-hovered': isHovered,
'has-bulk-actions': hasPossibleBulkAction,
} ) }
onMouseEnter={ handleMouseEnter }
onMouseLeave={ handleMouseLeave }
onClickCapture={ ( event ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of onClickCapture was introduced #59563 following-up similar changes for the grid layout at #59565.

As far as I understand:

  • we wanted to keep the onClick event for the titles (go to editor) while providing a way to select the whole row
  • using CTRL or META keys as modifiers provided a way to tell apart the user's intention

By reducing the title box surface, we have a larger TR area and the different surfaces become clearer targets. So, if we go this route (selecting the row without the modifiers), we could use onClick instead of onClickCapture. No difference that I can see other than a simpler mental model for onClick.

cc @jorgefilipecosta @mcsf in case there's some other context I'm missing. With what I know, I like selecting the whole row on click without the modifiers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the greedy onClickCapture approach was justifiable when it was predicated on event.ctrlKey || event.metaKey, otherwise it immediately becomes a blunt tool. Case in point, from the current branch:

Screen.Recording.2024-03-14.at.13.54.55.mov

There are tradeoffs to make here. Some approaches:

  • Simplify, get rid of this trick, expand the title box area, and listen to regular (non captured) click events on the title box. Get rid of the fancy ctrl/meta click behaviour.
  • Keep both side by side: ctrl/meta catches any click on the row, ignoring any underlying elements, while regular clicks with no modifiers will be caught on the title box (but no wider). After very short consideration, this is what I'd try first.
  • Go for the more complex solution without title boxes. For instance:
    • Ctrl/meta-clicks anywhere on the row should be caught at capture time, trigger a selection toggle, and stop event propagation.
    • Regular clicks anywhere on the row will need to consider whether more granular listeners were triggered (i.e. whether the user has clicked on any interactive element inside the row, like a button). There some ways to do this (e.g. inspecting all DOM nodes between event.target and event.currentTarget to find attached listeners), but I don't know how robust they are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep both side by side: ctrl/meta catches any click on the row, ignoring any underlying elements, while regular clicks with no modifiers will be caught on the title box (but no wider). After very short consideration, this is what I'd try first.

To clarify what I meant here: have two adjacent handlers:

<tr
  onClickCapture={ e => {
    if (e.ctrlKey || e.metaKey) {
      // Same as before
      e.stopPropagation(); e.preventDefault(); maybeToggle();
    }
  }
>

  ...
  <TitleArea
    onClick={ () => /* regular click handler to toggle */ }
  />

</tr>

if ( event.ctrlKey || event.metaKey ) {
event.stopPropagation();
event.preventDefault();
if ( ! hasPossibleBulkAction ) {
return;
}
onClick={ () => {
if ( document.getSelection().type !== 'Range' ) {
if ( ! isSelected ) {
onSelectionChange(
data.filter( ( _item ) => {
Expand Down Expand Up @@ -337,9 +331,20 @@ function TableRow( {
</td>
) ) }
{ !! actions?.length && (
<td className="dataviews-view-table__actions-column">
// Disable reason: we are not making the element interactive,
// but preventing any click events from bubbling up to the
// table row. This allows us to add a click handler to the row
// itself (to toggle row selection) without erroneously
// intercepting click events from ItemActions.

/* eslint-disable jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events */
<td
className="dataviews-view-table__actions-column"
onClick={ ( e ) => e.stopPropagation() }
>
<ItemActions item={ item } actions={ actions } />
</td>
/* eslint-enable jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events */
) }
</tr>
);
Expand Down
Loading