Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
If block instances can be removed from /structure/block-layout with the 'remove' action we should expose that as a contextual link as well.
A default block
A custom block
Proposed resolution
Add the "remove" contextual link to blocks.
Remaining tasks
Write patch
User interface changes
All blocks get a "remove" contextual link.
API changes
-
Data model changes
-
Comment | File | Size | Author |
---|
Issue fork drupal-2839558
Show commands
Start 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:
- 9.3.x compare
- 2839558-remove_blocks_link changes, plain diff MR !702
Comments
Comment #2
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #3
tim.plunkettI have to assume that you have #2826728: Block layout action removes instance, but contextual link deletes all instances. running locally, because there are no "Remove" contextual links for blocks yet.
In HEAD it says "Delete" because only custom blocks can be deleted.
I think this and that other issue should be combined into a feature request for "Add a 'Remove' contextual link to placed blocks" or similar.
Comment #4
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #5
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #6
tim.plunkettComment #12
Chris CharltonToo bad this little guy got stalled. Consistency is a platform asset. :)
Comment #13
tim.plunkettFeel free to review the patch!
Comment #14
Chris CharltonI did. It's pretty straightforward. It failed tests, so I assume that's a blocker? Or no?
Comment #15
tim.plunkettIt failed tests because the tests were brittle. Those can be fixed.
But did it work when you tried it out?
Comment #20
mohit_aghera CreditAttribution: mohit_aghera at QED42 commented@tim.plunkett
This patch is working as expected and adding "Remove" contextual link for the blocks.
I've also added additional test cases where we are rendering static blocks like "powered by Drupal".
That block has two links as it is static.
Comment #21
guilhermevp CreditAttribution: guilhermevp at CI&T commentedPatch works as intended. Adding video evidence.
Comment #22
djsagar CreditAttribution: djsagar at OpenSense Labs commentedHi @mohit_aghera,
I applied your patch but not adding "Remove" contextual link for the blocks.
i created custom block and placed but didn't find any changes.
please review the attachment.
Thanks!
Comment #23
mohit_aghera CreditAttribution: mohit_aghera at QED42 commented@djsagar
I am able to see the links on the blocks created via "block_content"
Can you please share the exact steps?
Comment #24
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #20 and it works fine.
Before patch:
After patch:
RTBC +1
Comment #25
vakulrai CreditAttribution: vakulrai as a volunteer and at QED42 for QED42 commentedI verified The patch #20 and its working as expected. #22 I think the problem you are facing is because of the block cache, try clearing it once.
Added a screenshot.
Thanks
Comment #26
hinal05 CreditAttribution: hinal05 as a volunteer and at QED42 for Drupal India Association commentedApplied patch #20 and it's working fine for me.
For #22 comment, I think this issue is replicated after applied patch, old block not adding "Remove" link. We need to add new custom block then it will replicating.
You can check this thing in attached GIF.
Comment #27
Madhu kumar CreditAttribution: Madhu kumar as a volunteer and at Zyxware Technologies commentedpatch #20 applied cleanly and working as expected , and have a "remove" contextual link ,screenshot added for reference
Comment #28
meghasharma CreditAttribution: meghasharma as a volunteer and at QED42 for Drupal India Association commentedPatch #20 applied cleanly and it is working well.
Reviewed the #26 and #27 screenshots, Looks good
screenshot for reference.
Added "remove" action contextual link
Marking as RTBC
Comment #29
tim.plunkettSimilar to the "Configure block" contextual link (and to the "Remove block" one already in Layout Builder), I think both of these should be "Remove block"
This is very trick test code. Could this instead be added to a FunctionalJavascript test and use a normal assertion?
Comment #30
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedThanks @tim.plunkett
I've updated the title and test cases so that it uses FunctionalJavascript tests.
Comment #31
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedComment #33
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedComment #34
mohit_aghera CreditAttribution: mohit_aghera at QED42 commented@tim.plunkett
I've refactored tests to separate classes.
Contextual links test in
BlockFilterTest
was failing for theClaroBlockFilterTest
tests.The reason is, for the claro theme, we are disabling contextual links for a few blocks.
Update
Tests are passing on local.
Here is the quick output.
Triggering the test build again.
Comment #36
tim.plunkettThis needs to wait for the contextual links to render before asserting. It's passing locally for you because you are running a single test. The testbot runs 15 test concurrently, so the gap between page render and contextual links render is longer.
Something like
(selector might need adjusting)
Additionally, can we assert that the page contains what we want without needing all of the markup hardcoded in the assertion?
Comment #37
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedUpdating the suggestions mentioned in #36
Comment #39
benjifisherPlease do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.
Comment #40
alexpott#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.
Comment #41
larowlanThis feels like a feature request to me, and @tim.plunkett said the same in #3
Comment #42
larowlanComment #45
catchRestoring status after HEAD was broken.
Comment #46
webchickCode looks straight-forward enough! Here's a review from the UX side.
The screenshot in the issue summary shows "Remove" as the last option, and that also tracks with what I'd expect, since it is very likely to be the least used option.
And yet... Here it is in the middle, on a menu block?
Upon clicking "Remove block," it busts you out of the front-end into a standalone confirm page using the admin theme:
This is at odds with what Quick Edit is intending to do, which is keep all of your admin functions on the front-end. Is there a reason a dialog wasn't used here for the confirm box instead?
And finally, the wording there is super spooky... It makes it sound like you're removing the User account menu block from the block system entirely, but you're really only removing its placement from this one region. And it can easily be undone. Do we have options to rephrase here? For example:
"Are you sure you want to remove the block User account menu from the Secondary menu region?
This will remove the block placement. You will need to place it again in order to undo this action."
Comment #48
tim.plunkettThanks for the review! There are three points of feedback: order of the links, redirecting to admin theme, and the wording of the form.
1)
Contextual links have a
weight
property, but they also have agroup
property. The weight only affects their order within their group.The "Configure block" and new "Remove block" links have a
group
ofblock
.The "Edit menu" link has a group of
menu
.This means that no matter how high of a weight we set for the new link, it will still be below the "Edit menu" link.
In theory we could implement
hook_contextual_links_view_alter()
to move it to the bottom... But that's really heavy-handed.2)
Currently all of the contextual links redirect to a page in the admin theme.
The Settings Tray module was working to stop doing this, but it's
a) not installed by default
b) does not fix these links to use it
We don't use dialogs for any of this right now.
I opened #3215209: After clicking "Remove block" contextual link, open form in off-canvas dialog as a follow-up for this
3)
I changed the wording of the form to match your suggestion. That's the only code change currently...
NOTE: I switched from patches to a new MR!
Comment #49
phenaproximaTook a look at this and I think that @webchick's feedback has been addressed. Restoring RTBC!
Comment #51
webchickTim walked me through this again today and lookin' good! All feedback addressed.
Committed and pushed to 9.3.x. Thanks!