SAM is great for allowing you to add items one by one without a clunky interface.
But what when you want to remove one item?
An experienced user knows that clearing the fields so Drupal considers it "empty" allows you to remove an item. But it's not obvious and, with complex widgets, it's tedious.
So let's add an explicit "Remove" button to remove one item.
Add a "Remove" button via JS that clears the widget and hides it.
I tried first to integrate with https://www.drupal.org/project/multiple_fields_remove_button, but sadly wasn't able to.
That module takes care of it server-side, but the button they add does everything in an ajax callback. I couldn't find a way to react to those changes while ensuring the "Remove" button triggered them.
So ended up including it client-side in SAM.
Patch, review, discuss.
A "Remove" button is added.
TBD
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | button-alignment.png | 210.76 KB | marcoscano |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
penyaskitoComment #4
penyaskitoFound an issue, the button is not showing when we are already at the limit of items (e.g. cardinality 3 and I have 3 items).
Comment #5
penyaskitoWe just needed to process everything no matter if all items were not empty.
Comment #6
penyaskitoFixed another bug when we remove the last item, we want to show the button and the number of pending items again.
Comment #7
marcoscanoFirst of all, I am 👍 to include this, thanks! (I too have been asked by editors "how do I empty an existing value?" :) )
I left a minor comment in the MR, doesn't look like we need too many changes IMO.
However, in testing this on Tugboat, I see a couple things we probably want to address:
1) In testing with the seven admin theme, the button displays uncentered:
would it be worth adding a little bit of CSS to try to make this look good(-ish) in all admin themes?
2)
I got an error in the console the first time I tested it(the error appeared when revealing a bunch of values and then removing some of them), but now I can't reproduce it... 🤯 I need to go for now but I'll come back and try to reproduce it again.Maybe we can add a few assertions to https://git.drupalcode.org/project/sam/-/blob/1.0.x/tests/src/Functional... to add test coverage for the new feature? That would be great.
Thanks!
Comment #8
penyaskitoThanks for the feedback!
Definitely needs tests. I've been seeing weird looping issues in our playwright tests, but that might be an issue on the tests or playwright itself.
Comment #9
penyaskitoI think this is ready now. Needs tests though.
Comment #10
penyaskitoAdded tests.
Drupal 9 is end of life while this issue was fixed, so not sure if still a requirement? Being fair I only tested with Claro.
Comment #11
marcoscanoThanks for working on this! (and apologies for the delay in reviewing it)
I think this is very close. To me the only remaining issues are:
1- Add support for CKEditor5 textareas
2- Adjust the theming in Seven. (I know D10 no longer includes it, but there are a lot of D10 sites still using it in the contrib namespace, it would be uncool to break those sites... :) )
Thanks!
Comment #13
minirobot commentedI added ckeditor 5 support based on @penyaskito's notes.
It looks like the remove button alignment was fixed by @penyaskito already.
Comment #14
minirobot commentedComment #15
penyaskitoI even forgot that I added tests, thought those were still needed!
I've no strong feelings about ckeditor explicit coverage.
Will defer on Marcos if those are requested. If he doesn't require them this LGTM.
Comment #17
marcoscanoGreat work @penyaskito and @minirobot! I don't believe we need extra tests for now.
Merged and tagged 1.2.0 with it.
Thanks! 🎉