Add: Bulk actions API to dataviews and an initial bulk trash action. #56476
Conversation
jorgefilipecosta
requested review from
ntsekouras,
youknowriad and
oandregal
jorgefilipecosta
added
the
[Feature] DataViews
|
Size Change: +1.02 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Just noting IMO the best place to implement bulk actions would be the templates list, since not all items have the same available actions. This use case will probably inform us better of the API we need to introduce. What do you think? |
jorgefilipecosta
force-pushed
the
add/bulk-actions
branch
from
12925b1
to
eeace64
Compare
|
Thank you for the PR Jorge! I'd expect the bulk actions container to position itself at the top when the list is bigger than the viewport. Screen.Recording.2023-11-27.at.3.36.05.PM.movAnother thing is that we could add the Would it be hard to add a bulk action(delete) in templates too? As I mention above:
|
| @@ -307,11 +330,14 @@ export default function PagePages() { | |||
| paginationInfo={ paginationInfo } | |||
| fields={ fields } | |||
| actions={ actions } | |||
| bulkActions={ bulkActions } | |||
There was a problem hiding this comment.
Have you explored if we can have a single actions API that also support an action to be a bulk one?
There was a problem hiding this comment.
Very good question 👍 I think the way a bulk action is done should be its own function and not a single action called multiple times ( in the future bulk actions may have special endpoints), but as part of the normal actions API we can have a bulkCallback that specifies an action supports being applied in bulk and how that is done. We may also be able to reuse the isEligible. Would you prefer this alternative implementation? If yes I can give it a try.
There was a problem hiding this comment.
An exploration at this point where we introduce a new API would be useful I think. I'm not opposed to this approach, but we need to explore if we can combine them. It could lead to a more elegant API or a more complex one 😅
There was a problem hiding this comment.
I feel like we should only have one API personally. An action is just a bulk action that applies to a single item. I believe we can have flags about whether an action can apply to multiple items or not.
There was a problem hiding this comment.
I feel like we should only have one API personally. An action is just a bulk action that applies to a single item. I believe we can have flags about whether an action can apply to multiple items or not.
This may be a good way to think about it, implement the action as a bulk action but have a special condition where if it is just one item we show more user-friendly messages, etc. I will give it a try.
| @@ -157,6 +158,20 @@ export default function PagePages() { | |||
| totalPages, | |||
| } = useEntityRecords( 'postType', postType, queryArgs ); | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
What is this code doing?
There was a problem hiding this comment.
I added a comment, it removes any selected pages that are no longer in the list of visible pages. For example, remove a deleted page from the set of selected pages.
There was a problem hiding this comment.
Hm.. so this is something every consumer should have, right?
There was a problem hiding this comment.
Yes if we follow the current direction of being the consumer managing the selection state, the consumer needs this logic.
jorgefilipecosta
force-pushed
the
add/bulk-actions
branch
3 times, most recently
from
7212676
to
25f4c2b
Compare
|
Hello @ntsekouras, @youknowriad thank you for the reviews! This PR has been updated and there is no longer a separation between the API for bulk actions and actions. Additionally, I have submitted a separate PR to implement bulk actions for templates, which is functional and can be merged after this one #56615. It did not require any API changes and the implementation was straightforward. As per your suggestion, I have also added the "some selected" state. Regarding the bulk actions no longer being visible if the viewport is small, the issue is a bit complex. I am currently using a popover to display the information above the pagination. In order to ensure that the popover is always visible, one possible solution would be to add a property to the popover that makes it visible at the edge of the viewport if the element it references is outside the viewport. However, I am unsure if such a property would make sense cc: @ciampo. Another alternative would be to not use a popover and try to position things with CSS. This would mean things would not be above the pagination, they would be positioned always at the edge of the viewport. I guess we can leave this discussion for a separate issue where we polish the UI. |
| ) | ||
| ); | ||
| } | ||
| }, [ pages, selection ] ); |
There was a problem hiding this comment.
This can be done automatically before calling the. onChangeSelection callback or when calling the onView callback with a page change.
There was a problem hiding this comment.
Hello @youknowriad, I am uncertain about how to approach the issue at hand. It seems that if a page is deleted, neither the onChangeSelection nor the onView functions are triggered. Essentially, we require a mechanism to automatically remove the deleted page (or page that stopped being visible because of a state change) from the selection set if it was previously included.
There was a problem hiding this comment.
This is another point that I don't get why the selection handling is at the consumer level and not handled internally, but 🤷
There was a problem hiding this comment.
I don't mind internal selection state but we need an onChangeSelection callback to adapt the UI in site-by-side view.
For the pages removed externally, what's wrong with keeping an outdated selection object with things that are not visible?? Is that bad.
There was a problem hiding this comment.
I'm not convinced the preview selection and bulk action selection are the same thing. This is how it looks like in practice when we do so:
Gravacao.do.ecra.2023-12-04.as.09.55.13.mov
My suggestion would be to separate concerns:
- Leave the current
selection/setSelectionfor the list view and iterate on the API in a separate PR. I've started exploring an alternative that uses aonClickPreviewcallback at DataViews: iterate on list view #56746 - In this PR, make DataViews absorb the bulk edit selection.
For me, bulk action selection is such a fundamental operation that the consumer should not have to deal with it. It seems that the concerns shared (have a controlled&uncontrolled selection) would be solved if we do not conflate preview & bulk action selection.
There was a problem hiding this comment.
Personally I have a feeling that in views where there's a preview, we want to show some kind of messaging (or stack in the preview) while the bulk actions should be visible as well. In the screenshot above, I'm sharing what "Mail" app does in Mac OS.
If you select a single email, the top bar shows the "actions" for that email and shows the preview on the right but if you select multiple emails, it shows a stack on the right and the "actions" are updated, the ones that apply to a single email are disabled so they in fact become bulk actions.
So I think there's value in treating the selections as similar. I do think the way selection is performed can be different from a view to another: for instance in table view, selection happens using the checkboxes while in the list view it happens using click or "ctrl + click to multi select"
There was a problem hiding this comment.
I see your point. I agree selection state could be something shared across all views – even if they work differently UI wise. I'd still argue that we don't want the selection to be controlled in the list view. To me the question is: who should handle the selection affordance in the list view: a field or via the view itself?
The current implementation makes the selection affordance to be provided by a field (through the <Link /> component). Though the mockups for the list view that I've started to implement at #56746 make the selection affordance to be provided by the view (structural li element of the view, not a link rendered by a field).
By having the selection affordance in the view, we don't need a controlled selection, we just need to relay the selection to the consumer, which we can do by a callback prop:
<DataViews
onItemsSelected={ ( { itemsSelected } ) => { /* handle the preview or any other thing * / } }
/>There was a problem hiding this comment.
I think the selection should definitely be updated down => top (dataviews component calling a callback onSelectionChange or onItemsSelected if you prefer). If a field is updating the outside "selection" object directly, that's a bug (regardless of whether we're maintaining the state internally or externally) for me.
That said, I do see some need of top => down selection change potentially in the future (thinking about things like undo/redo...) It's probably too early to think about that though. So whether it's controlled or uncontrolled today doesn't matter much.
There was a problem hiding this comment.
If a field is updating the outside "selection" object directly, that's a bug (regardless of whether we're maintaining the state internally or externally) for me.
This is how it works today: the title field sets the selection. 🤔
So, if I understood correctly, does it sound like we agree to the following plan:
- selection work the same for preview & bulk actions
- absorb selection as part of dataview's responsibility, not the consumer's
- dataviews should expose an
onSelectionChangecallback for the consumer
Given the current implementation for the preview, I'm not sure that we'd be able to make dataviews absorb selection fully in this PR. If @jorgefilipecosta feels confident, go ahead. Otherwise, I guess we could ship this as it is (selection controlled by the consumer) and revisit when something like #56746 lands.
Does that sound sensible?
There was a problem hiding this comment.
yes,
I think the issue is that instead of calling the setSelection state, the field should receive a setSelection callback in the render function and call it. Fields could be defined in very different places than where the selection state is hold (either internally or externally)
Maybe Let's get some design feedback though --cc @jameskoster @SaxonF |
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| const { | ||
| DropdownMenuV2Ariakit: DropdownMenu, |
There was a problem hiding this comment.
We shouldn't revert the recent changes here. Maybe a bad rebase? By seeing this it makes me wonder if more changes in other files could slip in that shouldn't? 🤔
There was a problem hiding this comment.
It was a rebase issue, it is fixed. I did a check in other files and did not notice other cases.
| setSelection( data.map( ( { id } ) => id ) ); | ||
| } | ||
| } } | ||
| /> |
There was a problem hiding this comment.
This checkbox is unlabelled.
There was a problem hiding this comment.
A label was added 👍
| ); | ||
| } | ||
| } } | ||
| /> |
There was a problem hiding this comment.
This checkbox is unlabelled.
There was a problem hiding this comment.
A label was added 👍
|
Chiming in here from #56615 The selection checkboxes are unlabelled. When omitting the @ciampo this is one more case that proves these input components including That said, in this specific case we may need to handle the labelling in some special way.
I'd like to thank everyone for the efforts on the Data Views implementation, I do realize it's a big task. I'd think this is a great opportunity to build something better than what we currently have in core. For the core tables, for a long time we could only try to implement small improvements but we couldn't rebuild everything from scratch because of backward compatibility issue. Now that we're starting fresh with the editor Data Views, this is the time to get it right since the begining. I'd love to hear thoughts from other accessibility specialists Cc @joedolson @alexstine @andrewhayward |
|
Why aren't you using react table selection api? |
| @@ -249,6 +253,54 @@ function ViewList( { | |||
| } | |||
| return column; | |||
| } ); | |||
| if ( selection !== undefined ) { | |||
| _columns.unshift( { | |||
There was a problem hiding this comment.
Is there any reason why you choose this approach instead of more simple one, using react table api?
I mean the same code here can be something like this:
_columns.unshift( {
header: ( props ) => {
return (
<CheckboxControl
__nextHasNoMarginBottom
checked={ props.table.getIsAllRowsSelected() }
indeterminate={ props.table.getIsSomeRowsSelected() }
onChange={ () =>
props.table.toggleAllRowsSelected()
}
/>
);
},
id: 'selection',
cell: ( props ) => {
return (
<CheckboxControl
__nextHasNoMarginBottom
checked={ props.row.getIsSelected() }
onChange={ props.row.getToggleSelectedHandler() }
/>
);
},
enableHiding: false,
width: 40,
} );
There was a problem hiding this comment.
Hi @Tropicalista,
Thank you for sharing insights and the code that uses the react table selection API. However, we have decided not to use it for two reasons:
Firstly, we have other views that are not compatible with tanstack, such as the grid view, and we need the selection to work seamlessly across all views.
Secondly, we are not certain about our future plans regarding tanstack, so we prefer to avoid using many of their API's and keep our options open.
jorgefilipecosta
force-pushed
the
add/bulk-actions
branch
from
f6f00b1
to
c43ebb6
Compare
|
Hi @afercia, I added labels that reference the page being selected/unselected.
I'm not sure what other UI we could use besides their own table column but I'm going to cc: @SaxonF and @jameskoster in case they have some insights. At least for now we have proper labels.
This is an already existing issue and not part of this PR but I'm happy to address it. I guess the second column (name of the page) should not be a heading at all and just text/span, is that correct?
I'm going to cc:@SaxonF and @jameskoster again in case they have some insights in how to improve the usability and accessibility. Any other feedback is welcome. |
|
Flaky tests detected in 8e0ab84. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7099836059
|
|
I agree that we shouldn't be using heading inside the table. There are relatively few cases where headings really make sense inside a table, since you're combining two different types of semantics. It probably wouldn't cause any major problems, but if a screen reader user is navigating inside a table, then they'll use table navigation tools to move around, and not jump from heading to heading. I'm certainly game to explore alternate solutions to the checkbox being in its own column, but I also don't have a lot of great ideas. Since we can't use interactive content as a label - e.g., anything that's a link or button - there are going to be problems with using any of the existing visible content. |
| const [ isModalOpen, setIsModalOpen ] = useState( false ); | ||
| const actionTriggerProps = { | ||
| action, | ||
| onClick: () => setIsModalOpen( true ), | ||
| }; | ||
| const additionalProps = {}; | ||
| if ( action.isBulk ) { |
There was a problem hiding this comment.
We could normalize the items and always pass an array of items in actions.
There was a problem hiding this comment.
Right now we pass an array for bulk actions and a single item without an array for non-bulk actions. Normalizing means that we would pass an array even for actions that don't support bulk. Is that ok? I don't have a preference but I can follow the approach of always passing an array if we think it is ok.
There was a problem hiding this comment.
No strong opinions but I think it's better to normalize and always pass an array.
|
There has been some file & component renames in the last couple of days, which requires the current in-flight PRs to be updated. I've gone through those PRs and rebased them – hope I didn't miss anything! |
| @@ -274,7 +342,15 @@ function ViewTable( { | |||
| } | |||
|
|
|||
| return _columns; | |||
| }, [ fields, actions, view ] ); | |||
| }, [ | |||
| areAllSelected, | |||
There was a problem hiding this comment.
Is this necessary? It depends on selection and data changes, so do we have every case covered with them? Anyway, if we move the line where areAllSelected inside the useMemo we could remove it.
| @@ -472,6 +548,10 @@ function ViewTable( { | |||
| header.column.columnDef | |||
| .maxWidth || undefined, | |||
| } } | |||
| className={ | |||
| header.column.columnDef.className || | |||
There was a problem hiding this comment.
What is this for? I don't see any className prop in the pages fields' definition.
There was a problem hiding this comment.
I see now: it's for the selection field. I've commented above.
| }, | ||
| enableHiding: false, | ||
| width: 40, | ||
| className: 'dataviews-table-view__selection-column', |
There was a problem hiding this comment.
What do you think of scoping the CSS using the data-field-id that we already inject? We target the actions column using this mechanism. It'd be good to use the same mechanism for all.
Also, I don't like that we expand the Field API to allow className for fields (the change below to support this class opens it up for every field).
| } | ||
| /> | ||
| ), | ||
| id: 'selection', |
There was a problem hiding this comment.
This name, like the actions field below, is something that may conflict with consumers field IDs. What if we use a more scoped name such as dataviews-selection?
If the change for the actions becomes too convoluted, in can be done in a separate PR.
| width: 100%; | ||
| } | ||
|
|
||
| .dataviews-table-view__selection-column label { |
There was a problem hiding this comment.
Note the related comment above, this could be th [data-field-id="dataviews-selection"].
| <Toolbar label="Bulk actions"> | ||
| <div className="dataviews-bulk-actions-toolbar-wrapper"> | ||
| <ToolbarGroup> | ||
| <ToolbarButton onClick={ () => {} } disabled={ true }> |
There was a problem hiding this comment.
I'm not familiar with the toolbar component, so I may be missing context: I found it weird that we use an “inactive” button. Can we use a ToolbarItem instead?
|
@jameskoster Do note that if something is visually hidden, it will likely look like a keyboard trap when users enter it. You cannot do this with interactive control elements. Also note that this is the same issue that a lot of Gutenberg suffers from, all the appearing and disappearing is great visual UX but is a royal pain for people without sight. UI shouldn't come and go as much as it does on the modern web today. The suggestion is a WCAG violation though so sadly it's a non-starter anyway. |
|
I will try to keep each point in a separate comment. Bulk select mode reveals bulk action controlsI'd think that, typically, bulk actions are a flow users take once in a while, not that often. More often, users will work on a single image. As such, I'd would like to propose to have the Bulk select and bulk action controls not shown by default. Why we would want to have the checkboxes always there even when they are not needed? @joedolson @alexstine I'd greatly appreciate your thoughts.
Screenshots:
Showing the Bulk select and bulk action controls only when needed would have a few pros:
This is not unprecedented and seems to me it would be a common pattern for users. If done well, it can be accessible I think. |
Bulk actions toolbar
Yes, and I think the current UI for the bulk actions toolbar is less than ideal. First of all, it may be visually out of screen. That really depends on the screen size. Mobile is a concern too.
This isn't a great UX. There is a good reason why in the classic admin the bulk actions are placed both at the top and at the bottom of the table.
I'd totally second what @alexstine suggested. It feels natural and expected to have the bulk actions at the top. It is convenient to make them available also at the bottom. Of couser, basec on previous considerations about making the bulk actions show only when needed, the bulk actions toolbar may be shown only when in 'selection' mode. Let's please avoid floating / sticky / fixed UIs please, as they come with inherent usability and accessibility problems especially for keyboard users.
That's something we could explore but it would add some noise, I think. If we make the UI have as less tab stops as possible I think skip links would not be necessary. |
Icon-only buttonsLet's please avoid Icon-only buttons as much as possible. Icon-only buttons should only be used when there's no space to use visible text. For example, the 'View options' button:
Visually, the icon looks like a table or a grid. It reminds me of the 'Grid view' icon in the Media Library. As a user, I would expect htat it switches the view to a grid view. Instead, it opens a dropdown menu. The icon glyph really doesn't suggest the idea of 'View options'. The 'View options' themselves contain a variety of options that are different in nature and very difficult to communicate ny the means of an icon. Also, the icon is placed separately from other controls, it's 'in isolation' in a blank part of the UI with no other adjacent controls. For users with visual impairment, this icon is very difficult to see. The View options menuScreenshot: First of all, I woudl like to remind that the multi-level version of the dropdown menu still needs to be tested for accessibility and I would like to hear other a11y specialist thoughts about it. Also: please do not place the selected value inside the menu item this way, as it becomes part of the accessible name of the For example, this is not OK:
The accessible name becomes
Accessible name and selected value or state must be communicated separately. Visually, it may be ok but semantically this is incorrect. Please move the selected value / state out of the element with |
Dropdown menus with only one itemThis was previously noted in other issues and PRs. A dropdown menu that contains only one item is arguably a good UI. When there is only one item a dropdown is unnecessary. It's also obtrusive as it hides the item and it forces users to a double click for a control that should be in first function instead. Screenshot:
|
Could you expand on that? My thinking was that the toolbar would contain the bulk action tools, but each button would be disabled. Also, if you wouldn't mind linking to the reference that makes this approach a non-starter it would help my understanding of the problem(s). |
|
@afercia I agree with this.
This makes more sense then having to select a checkbox to eventually have bulk action controls appear. Selecting a button such as Enable bulk selection makes more sense. @jameskoster I've never been very good at finding WCAG requirements that specifically fail but let me try to explain this a little more.
Thanks. |
|
One problem with a dedicated "bulk select mode" is that it overly de-emphasises bulk actions on views where they are more prevalent, e.g. comment management. We also need to consider the upcoming "List view" which might employ a different selection pattern. I agree there should be a way for users (or views via config) to control the prominence of bulk actions, but wouldn't a view option for toggling the checkbox column be a more flexible solution? |
|
@afercia, related to the
The menu received a first round of review (including a focus on a11y) in #54939. It has since received more feedback across other PRs. Its underlying implementation is also based on
This is good feedback, and implementing the proposed solution (using the |
jorgefilipecosta
force-pushed
the
add/bulk-actions
branch
from
6c28d98
to
8e0ab84
Compare
|
Here's an alternative approach I'd like to get feedback on @alexstine. 
Do you think something like this could work? |
|
@jameskoster Seems fine to me. Then you can save the space in the table not showing checkboxes all the time. |
|
I'm not sure we can get around that one, the checkboxes probably need to remain to enable partial selection (e.g. select rows 4 and 8 of 20). I could potentially see a view option to toggle bulk editing though – when |
This PR adds a bulk actions mechanism to the dataviews and an initial bulk trash action.
It also adds an item selection mechanism to the list view.
Testing
Verify we can select all visible rows.
Verify we can deselect all visible rows.
Select some pages and verify it is possible to trash them all.
Verify the Deselect button works.
Screenshot