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.

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

Batch editing #6842

Open
wants to merge 25 commits into
base: v5/develop
Choose a base branch
from
Open

Batch editing #6842

wants to merge 25 commits into from

Conversation

bastianallgeier
Copy link
Member

@bastianallgeier bastianallgeier commented Dec 7, 2024

Changelog

Features

batch-delete

Enhancements

  • New Kirby\Cms\Pages::delete(array $ids) and Kirby\Cms\Files::delete(array $ids) methods to handle batch delete for multiple files in a collection.
  • New batch mixin for model sections (pages & files), which introduces the batch option and a computed property to check for supported layouts.
  • New selectable property for the <k-item> and <k-items> components. When set to true, the items show checkboxes and emit an select event, when the checkbox is clicked.
  • The files and pages sections introduce new delete API endpoints. Those endpoints can take a set of page or file ids and will delete each one. Errors will be caught and sent back in the details array of the final thrown exception.

Breaking changes

  • none

Docs

  • New batch option for pages and files sections
projects: 
  type: pages
  batch: true

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@bastianallgeier bastianallgeier marked this pull request as ready for review December 7, 2024 15:01
@bastianallgeier bastianallgeier requested a review from a team December 7, 2024 15:01
config/sections/mixins/batch.php Outdated Show resolved Hide resolved
config/sections/pages.php Outdated Show resolved Hide resolved
this.$panel.dialog.open({
component: "k-remove-dialog",
props: {
text: `Do you really want to delete ${this.selected.length} items at once? This action cannot be undone.`
Copy link
Member

Choose a reason for hiding this comment

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

Todo: Turn into i18n string.

@@ -231,6 +309,24 @@ export default {
this.searching = !this.searching;
this.searchterm = null;
},
onSelect(event, item) {
if (event.target.checked) {
Copy link
Member

Choose a reason for hiding this comment

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

Todo: Making this depend on the event (and more specifically a checkbox event) that many layers up from the actual input seems not ideal to me. Can this just accept the item as parameter and decide what to do based on whether the item is already in the selected array or not?

panel/src/components/Sections/ModelsSection.vue Outdated Show resolved Hide resolved
key: 'file.delete.multiple',
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Discussion: I of course understand why it is implemented like this currently, but for a core method in this class, it seems a bit unusual to swallow the actual exceptions and just throw this vague one. Not sure there is a better way.

@bastianallgeier bastianallgeier added this to the 5.0.0-beta.2 milestone Dec 7, 2024
@afbora
Copy link
Member

afbora commented Dec 8, 2024

I just wondering if it wouldn't be better if batch mode was enabled by default. I don't think it would be considered a breaking change to enable this feature. After all, it's a nice and useful feature.

@afbora
Copy link
Member

afbora commented Dec 9, 2024

Getting Not all pages could be deleted dialog error when I select a non-deletable pages. I have to reload the page after the error to see remains.

Ideal solution: shouldn't be able to select non-deletable models.

@afbora
Copy link
Member

afbora commented Dec 9, 2024

When batch is active, the status icon is still clickable. If you click the status icon, both the status dialog opens and batch selection works. A user may accidentally delete the model while trying to change the status while batch mode is active.

Ideal solutions:

  1. When the status icon is clicked, the status dialog should not open/work.
  2. If the status icon should work when batch mode is active, it should not trigger the batch mode checkbox.

@bastianallgeier
Copy link
Member Author

@afbora that's wrong. Reloading should not be necessary. Already blocking the checkbox would be ideal indeed, but also a lot harder to implement. That's why I went for a simpler first solution that just skips the ones that cannot be deleted during the request.

@afbora
Copy link
Member

afbora commented Dec 9, 2024

@bastianallgeier I've tested again and page is not reloading after the error.

@bastianallgeier
Copy link
Member Author

I've added table selection and it's working great

CleanShot.2024-12-10.at.12.19.59.mp4

@afbora
Copy link
Member

afbora commented Dec 10, 2024

@bastianallgeier Is it testable for bug-fixes?

@bastianallgeier
Copy link
Member Author

@afbora no, I still need to look into the exception issue.

@bastianallgeier
Copy link
Member Author

@afbora I've fixed it now and it's working fine for me.

@afbora
Copy link
Member

afbora commented Dec 10, 2024

@bastianallgeier Deletion doesn't work with paginated sections.

pages:
  type: pages
  batch: true
  limit: 2

@pReya
Copy link
Contributor

pReya commented Dec 11, 2024

I've added table selection and it's working great
CleanShot.2024-12-10.at.12.19.59.mp4

Sorry for a slightly Off Topic remark: But I see you're touching the "ID" column of table views and conditionally using them as a checkbox for multi-select. I know of multiple projects that are hiding the ID column (because it's the only way to get rid of this column as it's not configurable). Is there any way to hide the column and still be able to use this new multi-select possibility?

@bastianallgeier
Copy link
Member Author

@afbora can you give more context. What happens in that case?

@bastianallgeier
Copy link
Member Author

@pReya if the id column is disabled, it should still be visible when the select mode is on.

@pReya
Copy link
Contributor

pReya commented Dec 12, 2024

@pReya if the id column is disabled, it should still be visible when the select mode is on.

AFAIK the ID column can't be disabled. It needs to be hidden via CSS (see linked forum post). So what I'm trying to say is: While you're working on this – is there any chance, you can allow disabling/hiding it while still allowing multi-select? See also https://kirby.nolt.io/460

@afbora
Copy link
Member

afbora commented Dec 12, 2024

@bastianallgeier Nothing happened. Just getting Not all pages could be deleted error and no pages are deleted. Even if they are deletable pages.

@nilshoerrmann
Copy link
Contributor

Two questions regarding the selection layout:

  1. Looking at the cards layout, I was wondering if using colours to highlight the selection would be a good idea. E. g. adding a blue outline or even changing the card colour (thinking of how selections work in Finder or Explorer). Right now, because the checkboxes are not that visually attached to the cards, it's hard to see the current selection easily. The checkbox' active state seen in the screenshot makes this very clear: it looks like the checkbox is active, not the card – but the user is interacting with the full card and not this single checkbox.
  2. And regarding the table layout – I'm sure I already reported that a few years ago – I am always wondering: Why are interactions like dragging and now selecting attached to the first cell? Having the action inside a cell makes it look like a data entry actually. But again, we are interacting with the entire row and for the list layout the drag icon is visually prefixing the item already, hovering on the grey background. It would be great to have a consistent behaviour across layout variants here in my eyes.

A quick visualisation:

kirby-table

I'm really fond of the introduction of batch editing. But I'd personally prefer a bolder design for destructive interactions like this to clearify the impact of the action. That's why I'd like to suggest the inverted colours here.

grafik

@distantnative
Copy link
Member

@nilshoerrmann if you have some good tips how to implement drag handles outside of a <table> element, please let us know. We tried #4386 but so far without success.

@nilshoerrmann
Copy link
Contributor

@distantnative I would put the handle inside the first cell (which ever that is in the current table) and move it outside using translate. I haven't tested this right now but I think I have done that this way before.

@nilshoerrmann
Copy link
Contributor

I've just tried in the lab, quickly altering the CSS:

grafik

The only problem might be that you have to remove the overflow handling:

grafik

Not sure why it's there to be honest. So that would be interesting to know, if that really has to stay.

@nilshoerrmann
Copy link
Contributor

Oh, the overflow: hidden is for text shortening with an ellipsis of course (sorry, I didn't notice before because we usually remove that behaviour with custom CSS). In that case an additional wrapper would be needed inside the cell or it would be possible to to create an "invisible" cell at the start of the table (zero width, no background and borders) to place the drag handle inside and allow overflow for that cell specifically.

@nilshoerrmann
Copy link
Contributor

nilshoerrmann commented Dec 12, 2024

By the way, I've read in the linked issue about the issues with hover interactions: as long as you don't remove the handle from the content flow with display: none this works perfectly. Setting opacity: 0 by default, switching to opacity: 1 on row hover makes this visually consistent with the list layouts.

@nilshoerrmann
Copy link
Contributor

nilshoerrmann commented Dec 12, 2024

@distantnative If you apply the following CSS to https://lab.getkirby.com/public/lab/components/items/4_table the drag handle will be outside of the table with sorting functionality intact, showing on hover:

.k-table .k-table-index-column {
  /* ↑ this would need a dedicated first column with a different name, e. g. .k-table-sorting-column */
  overflow: auto;
  position: relative;
  width: 0;
  padding: 0;
}

.k-table .k-table-index-column .k-sort-handle {
  /* display: none; ← remove this */
  position: absolute;
  translate: -2rem;
  left: 0;
  top: 0;
  opacity: 0;
  width: 2rem;
}

.k-table thead th:first-child {
  position: initial;
}

.k-table .k-table-index {
  display: none; /* ← this removes the index number which would have to move to the second column instead */
}

.k-table .k-table-sortable-row:hover .k-sort-handle {
  opacity: 1;
}

In this case the first index column will be visually hidden. So it's a proof of concept that this works with a "ghost" column. Tested in Firefox and Safari.

There is a display error with images inside the table when sorting (outline cut off by image) but that's unrelated to the above styles.

@johannahoerrmann
Copy link

Another issue I see with placing the select in the ID cell (and thus hiding the number) is – numbers are very useful to see during batch edit if they are something editors actually work with. The number is the clearest and easiest indicator for specific rows, so I do see the use case of eg. jotting down or agreeing on numbers of the items to delete (or otherwise edit, once that's possible). Pretty much the exact opposite use case of what @pReya wrote, but the same issue (showing select in the space reserved for numbers).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants