Add: Bulk actions to dataviews with the new design. #57255
Conversation
jorgefilipecosta
added
[Type] Experimental
|
Size Change: +2.16 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/bulk-actions-to-dataviews
branch
from
74edd50 to
35605cc
Compare
jorgefilipecosta
requested review from
ntsekouras,
oandregal,
youknowriad and
jameskoster
|
I think the work here can pave the way for more elaborate designs in the future, but for now this feels like a good starting point that is lightweight and accessible. Visually the menu doesn't seem to match the latest DropdownMenuV2 designs, perhaps that will be fixed by a rebase? Could we try moving the 'Bulk edit' button to the right, just left of the 'View options' button?
I'm seeing the selection controls on the Pages list, should those be disabled altogether if there are no actions? |
jorgefilipecosta
force-pushed
the
add/bulk-actions-to-dataviews
branch
from
35605cc to
69257e6
Compare
|
I was kind of surprised to see the checkboxes and bulk actions kind of visible by default. I think I've expected to have to first click "bulk edit" to show the checkboxes and the bulk actions menu. Just wondering if this is something that has been discussed? |
|
In the "pages" dataviews, I can see the checkboxes and the bulk selection menu even though I don't see any bulk actions to perform. |
|
Probably a follow-up but did we think about the UI for bulk selection in "list" and "grid" layouts? |
|
Maybe I'm doing something wrong but clicking the "reset" or "delete template" bulk actions does nothing for me. |
jorgefilipecosta
force-pushed
the
add/bulk-actions-to-dataviews
branch
2 times, most recently
from
9bce3f4 to
1e1a01c
Compare
Sorry, I missed pushing a post-rebase fix I had locally (onSelect -> onClick on a component that changed) it should be working now. |
I will implement the other views bulk actions in separate PRs to keep this one smaller. As far as I know, there are no mockups yet. I expect both for list and grid pressing shift plus a click would trigger multi selection of elements that seems to be a common pattern. But I'm to other options. |
I believe we haven't discussed this option yet. The Bulk Edit menu triggers a dropdown, which could make it a bit confusing to display checkboxes as well. To use this option, we would first need to click on "Bulk edit", which would display the checkboxes along with an actions menu. When we click on the actions menu, the current dropdown will appear with the actions and options to select all or deselect all. We would also need a button like stop bulk edit to hide checkboxes again. We can also consider another option where we hide the checkboxes to reduce visual clutter, but display them when we hover the mouse over a row. |
I added a secondary style to the bulk edit button, it should now have a similar style to the story book sample.
The issue was fixed. |
Fixed 👍 |
A 'bulk edit' mode was explored early on (similar to the media library in wp-admin) but it didn't feel super compelling, mostly due to the friction it adds to the selection process. It was quite difficult to answer: "why not make those controls permanently available"? It also raised questions about other table features when such a mode was active, e.g. would it still be possible to perform inline edits/actions, filtering, etc? In short it solves some issues while introducing others. I would say that the checkboxes themselves feel quite prominent visually, and that perhaps overemphasises bulk actions. This could be a detail to visit separately, and there are a few options to explore there:
|
I guess for me the answer to this is that I don't see bulk editing as the primary action on the lists, and something you'd rarely don, only for advanced use-cases. So it seems counter intuitive for me for these controls to be so prominent. |
This button is not well spaced. Also, I noticed that there's a focus loss issue on both actions, maybe a good approach would be to focus the "bulk edit" button once the action is performed. Other than that, this seems fine to land as v1 |
jorgefilipecosta
force-pushed
the
add/bulk-actions-to-dataviews
branch
from
6408593 to
1e37b3e
Compare
Hi @youknowriad thank you for catching these issues, I think both of them were fixed. |
packages/dataviews/src/style.scss
Outdated
| .dataviews__filters-custom-menu-radio-item-prefix { | ||
| display: block; | ||
| width: 24px; | ||
| } | ||
|
|
||
| .dataviews-bulk-edit-button.components-button.is-secondary { | ||
| padding-right: $grid-unit-15 + 2px; |
There was a problem hiding this comment.
Why we need a special padding in the first place. For me, the issue seems to be that the flex behavior is "shrinking" the width of the button so instead of a custom padding (which result in a wrong balance for me btw), we need to use flex-string: 0
There was a problem hiding this comment.
Good one flex-shrink did the trick. I did not find the root cause of why the button was cut on the right and the padding was like a last resource to address the issue.
| action, | ||
| selectedItems, | ||
| setActionWithModal, | ||
| bulkActionsButtonRef, |
There was a problem hiding this comment.
I don't like this prop. In general I don't like passing refs and Dom elements as props because: Which can of ref is this, an object, a function?... Also, they're subject to timing changes and mutations.
A better API here could be onPerform or something like that. Something to be called when the action is done for instance.
That said, I do wonder: I believe modals and popover have a built-in behavior to restore the focus to the caller (button that opened the popover/modal), in this case it doesn't work natively, so I'm curious to know why the "focus return" don't work by default when the dropdown get closed. cc @ciampo
There was a problem hiding this comment.
I believe modals and popover have a built-in behavior to restore the focus to the caller
The Modal, when closed, will try to restore focus on the element that was last focused before opening the modal — ie. the menu item. Similarly, the DropdownMenu will try to restore focus on the menu trigger when closed.
Therefore, if we want to focus the dropdown menu trigger when closing the modal, that may be something that needs to be implemented in a custom way. An alternative that we could consider is to leave the dropdown menu open — in theory, the modal component should be able to restore focus correctly on the menu item when closed (see storybook example).
There was a problem hiding this comment.
The Modal, when closed, will try to restore focus on the element that was last focused before opening the modal — ie. the menu item. Similarly, the DropdownMenu will try to restore focus on the menu trigger when closed.
I guess in my mind, when I click the trigger it should get focused and then open the modal so it's the last item that was focused before the modal is opened.
But now that you bring this up, I remember some discrepancies between browsers and not all of them have the "focus on click" behavior. Which means that maybe it's because of me testing on Safari which makes me wonder whether we should bother about fixing this issue (should we just revert the commit?) as this is the expected behavior there. I'll check in chrome (before the commit) to confirm.
There was a problem hiding this comment.
I went ahead and tried to implement an alternative solution that doesn't rely on bulkActionsButtonRef at all (and keeps the dropdown menu open in the background when rendering the modal). Could this work?
Click to expand
diff --git a/packages/dataviews/src/bulk-actions.js b/packages/dataviews/src/bulk-actions.js
index 9613ff37ee..3fa5db3d76 100644
--- a/packages/dataviews/src/bulk-actions.js
+++ b/packages/dataviews/src/bulk-actions.js
@@ -25,7 +25,7 @@ function ActionWithModal( {
action,
selectedItems,
setActionWithModal,
- bulkActionsButtonRef,
+ onMenuOpenChange,
} ) {
const eligibleItems = useMemo( () => {
return selectedItems.filter( ( item ) => action.isEligible( item ) );
@@ -33,11 +33,10 @@ function ActionWithModal( {
const { RenderModal, hideModalHeader } = action;
const onCloseModal = useCallback( () => {
setActionWithModal( undefined );
- // Focus the bulk actions button.
- if ( bulkActionsButtonRef?.current?.focus ) {
- bulkActionsButtonRef.current.focus();
- }
- }, [ setActionWithModal, bulkActionsButtonRef ] );
+ // Close dropdownmenu
+ // TODO: should be called only if the primary action of the modal is clicked
+ onMenuOpenChange( false );
+ }, [ setActionWithModal, onMenuOpenChange ] );
return (
<Modal
title={ ! hideModalHeader && action.label }
@@ -50,31 +49,23 @@ function ActionWithModal( {
);
}
-function BulkActionItem( {
- action,
- selectedItems,
- onMenuOpenChange,
- setActionWithModal,
- bulkActionsButtonRef,
-} ) {
+function BulkActionItem( { action, selectedItems, setActionWithModal } ) {
const eligibleItems = useMemo( () => {
return selectedItems.filter( ( item ) => action.isEligible( item ) );
}, [ action, selectedItems ] );
+
+ const shouldShowModal = !! action.RenderModal;
+
return (
<DropdownMenuItem
key={ action.id }
disabled={ eligibleItems.length === 0 }
- onClick={ async ( event ) => {
- event.preventDefault();
- if ( !! action.RenderModal ) {
- onMenuOpenChange( false );
+ hideOnClick={ ! shouldShowModal }
+ onClick={ async () => {
+ if ( shouldShowModal ) {
setActionWithModal( action );
} else {
await action.callback( eligibleItems );
- // Click the bulk actions button to close the dropdown and focus the button.
- if ( bulkActionsButtonRef?.current?.click ) {
- bulkActionsButtonRef.current.click();
- }
}
} }
suffix={
@@ -86,13 +77,7 @@ function BulkActionItem( {
);
}
-function ActionsMenuGroup( {
- actions,
- selectedItems,
- onMenuOpenChange,
- setActionWithModal,
- bulkActionsButtonRef,
-} ) {
+function ActionsMenuGroup( { actions, selectedItems, setActionWithModal } ) {
return (
<>
<DropdownMenuGroup>
@@ -101,9 +86,7 @@ function ActionsMenuGroup( {
key={ action.id }
action={ action }
selectedItems={ selectedItems }
- onMenuOpenChange={ onMenuOpenChange }
setActionWithModal={ setActionWithModal }
- bulkActionsButtonRef={ bulkActionsButtonRef }
/>
) ) }
</DropdownMenuGroup>
@@ -131,7 +114,7 @@ export default function BulkActions( {
selection.includes( getItemId( item ) )
);
}, [ selection, data, getItemId ] );
- const bulkActionsButtonRef = useRef();
+
if ( bulkActions.length === 0 ) {
return null;
}
@@ -143,7 +126,6 @@ export default function BulkActions( {
label={ __( 'Filters' ) }
trigger={
<Button
- ref={ bulkActionsButtonRef }
className="dataviews-bulk-edit-button"
__next40pxDefaultSize
variant="secondary"
@@ -164,19 +146,14 @@ export default function BulkActions( {
>
<ActionsMenuGroup
actions={ bulkActions }
- data={ data }
- selection={ selection }
- getItemId={ getItemId }
- onMenuOpenChange={ onMenuOpenChange }
setActionWithModal={ setActionWithModal }
selectedItems={ selectedItems }
- bulkActionsButtonRef={ bulkActionsButtonRef }
/>
<DropdownMenuGroup>
<DropdownMenuItem
disabled={ areAllSelected }
- onClick={ ( event ) => {
- event.preventDefault();
+ hideOnClick={ false }
+ onClick={ () => {
onSelectionChange( data );
} }
suffix={ data.length }
@@ -185,8 +162,8 @@ export default function BulkActions( {
</DropdownMenuItem>
<DropdownMenuItem
disabled={ selection.length === 0 }
- onClick={ ( event ) => {
- event.preventDefault();
+ hideOnClick={ false }
+ onClick={ () => {
onSelectionChange( [] );
} }
>
@@ -199,7 +176,7 @@ export default function BulkActions( {
action={ actionWithModal }
selectedItems={ selectedItems }
setActionWithModal={ setActionWithModal }
- bulkActionsButtonRef={ bulkActionsButtonRef }
+ onMenuOpenChange={ onMenuOpenChange }
/>
) }
</>
There was a problem hiding this comment.
Actually, I just did more testing and here's my findings:
First issue
For the "reset template" action, there's no modal, but what's happening there is that the "dropdown" stays open but the action become "disabled" which means there's a focus loss. I believe there are two things we should do here:
- The simple solution is to just close the dropdown when the action is done
onActionPerformedor something which should in theory restore the focus to the trigger. - The second thing that we need to do IMO (even if it's probably not necessary once the first solution is implemented) is that "disabled" menu items in the dropdown menu shouldn't be "disabled" entirely, they should use the
aria-disabledtrick. (Equivalent to the__experimentalIsFocusableprop on the Button component). This will allow the buttons to stay focusable and prevent focus loss even though the action is disabled.
Second issue
For the "delete template" action, there's a modal, so there are nested dialogs basically. And in this case, everything closes and there's a focus loss. I'm not entirely sure what's happening here so I have no solutions to offer for this case yet, but my guess is that with the "aria-disabled" trick that I suggest above, there's a small chance of this issue being fixed as well.
I guess what I'm saying is that we shouldn't try to add custom code to solve these focus loss issues for this particular UI, instead we should try first to address the systemic issues that I raise above. Does that make sense?
There was a problem hiding this comment.
And for the second issue, you're right as well, the popover is closed when the modal is opened. I'm not entirely sure if we do this on purpose or not but could this be the result of "focus loss". Since the focus is not on the popover anymore, it just closes itself. In other words, our popovers/dropdowns might not support nested dialogs being focused properly yet.
There was a problem hiding this comment.
- It actually becomes a "div" and not a button anymore. Weird. So it's a similar issue but different trigger. so yes, not the aria-disabled trick.
- The activeElement is the dropdown wrapper in my tests, so it seems there's some code in place already that handle a focus fallback or something.
I think all dropdown menu items are actually div[role="menuitem"] elements.
Regarding the activeElement:
- when using the keyboard to click the "Deselect" item, focus stays on the item
- when using the mouse, focus moves to the menu wrapper
So I believe that focus is already being handled correctly by the underlying ariakit implementation.
And for the second issue, you're right as well, the popover is closed when the modal is opened. I'm not entirely sure if we do this on purpose or not but could this be the result of "focus loss". Since the focus is not on the popover anymore, it just closes itself.
Closing the dropownmenu when opening the modal is indeed the cause for focus loss. If we kept the dropdownmenu open, the menu item that was clicked to open the modal would receive focus when the modal gets closed.
In other words, our popovers/dropdowns might not support nested dialogs being focused properly yet.
IMO, the better approach (also UX-wise) is to keep the dropdown menu open under the modal. Doing so should also allow the components to handle focus correctly without extra code:
- Modal would handle focus restoration when dismissing the modal by focusing the correct dropdown menu item
- If we want to force the dropdown menu to close, setting its
openprop tofalseshould move focus automatically on the dropdownmenu trigger. So we should be already able to cover
When used that way, our components should already handle focus correctly.
There was a problem hiding this comment.
IMO, the better approach (also UX-wise) is to keep the dropdown menu open under the modal
I agree but my question was: Is that even possible with our current components. I believe most of our "popovers"... close themselves when they lose focus to a child modal/popover but you say otherwise, so that's news for me :)
There was a problem hiding this comment.
Yup! The new DropdownMenu menu doesn't use our underlying Modal component, and can be definitely be kept open under a Modal.
I hope that the solution proposed above can be a good starting point for a better UX
There was a problem hiding this comment.
I applied the patch suggested by @ciampo and implemented the remaining todo specified there. Thank you a lot for providing a detailed patch! Things seem to be working well.
The current behavior is:
- When an action without modal is executed the menu closes and the focus goes to bulk editor button.
- When an action opens a modal the menu keeps open if the modal is closed the focus goes to the button that triggered the modal, if the action in the modal is executed the menu gets closed and the focus goes back to the bulk edit button.
@youknowriad @ciampo let me know if there is any other update required.
I'd agree bulk actions are not always primary, but I don't know that they're 'only advanced'. There are some fairly common flows where bulk actions are situationally primary, e.g. moderating comments, deleting inactive plugins, updating tags or categories. I'd still like to explore fine tuning the visual prominence before introducing a dedicated mode, there are many things to try there. |
jorgefilipecosta
force-pushed
the
add/bulk-actions-to-dataviews
branch
from
d2dfac6 to
3ce5184
Compare
|
@jorgefilipecosta my style changes from yesterday seem to have vanished 🤔 Is there any way to retrieve them or must I re-do that work? Edit: Never mind, I re-did it :d |
I'm sorry I'm not sure what happened probably some rebase issue 😞 thank you for re-adding the commit 🙇 |
|
All the blockers have been addressed thank you all for the feedback, reviews, commits, and suggestions. I will merge this PR so we can continue with follow-ups and related work and avoid other big rebases. However, any feedback is still welcome and will be applied as a follow-up. |
Supersedes: #56476, #56615
This PR proposes a bulk actions implementation using the DropDownMenuV2 component trying to follow the design proposed at #56476 (comment).
For now, bulk actions are implemented only for templates.
If multiple items are selected, each one supporting different bulk actions, we show the number of elements the action applies to and only apply the action to the eligible elements.
The actions API was expanded with a supportsBulk option. We need to know if the action supports a bulk option or one to decide if it appears in the bulk edit menu. E.g.: A see revisions action would not make sense under bulk edit.
The actions API callback and Render modal were also changed so the items are an array (support multiple items instead of just one).
Screenshots or screencast