-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Data views: Add click-to-select behavior on table rows #59803
Changes from all commits
b9064d4
4880f5d
146418f
e3766a2
6db4bb9
e2bf36c
242966d
3541f13
fa91886
64c4967
be1856a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,7 +237,6 @@ function TableRow( { | |
data, | ||
} ) { | ||
const hasPossibleBulkAction = useHasAPossibleBulkAction( actions, item ); | ||
|
||
const isSelected = selection.includes( id ); | ||
|
||
const [ isHovered, setIsHovered ] = useState( false ); | ||
|
@@ -250,22 +249,28 @@ function TableRow( { | |
setIsHovered( false ); | ||
}; | ||
|
||
// Will be set to true if `onTouchStart` fires. This happens before | ||
// `onClick` and can be used to exclude touchscreen devices from certain | ||
// behaviours. | ||
const isTouchDevice = useRef( false ); | ||
|
||
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 ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of As far as I understand:
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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the greedy Screen.Recording.2024-03-14.at.13.54.55.movThere are tradeoffs to make here. Some approaches:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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; | ||
} | ||
onTouchStart={ () => { | ||
isTouchDevice.current = true; | ||
} } | ||
onClick={ () => { | ||
if ( | ||
! isTouchDevice.current && | ||
document.getSelection().type !== 'Range' | ||
) { | ||
if ( ! isSelected ) { | ||
onSelectionChange( | ||
data.filter( ( _item ) => { | ||
|
@@ -337,9 +342,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> | ||
); | ||
|
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#60191