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 description
When the user is in edit mode and clicks on a custom block contextual link there are two “quick edit” links in the list.
Proposed solution
Remove the second quick edit link since the first already opens the tray and triggers quick edit of the text.
Update the labels on 1 or both of the links to differentiate them to the user.
Remaining tasks
Pick the labels for each link.
User interface changes
1 or both links labels will be changed
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#35 | interdiff-30-35.txt | 694 bytes | gaurav.kapoor |
#35 | 2786193-35.patch | 4.95 KB | gaurav.kapoor |
#30 | 2786193-30.patch | 4.95 KB | tedbow |
#27 | 2786193-27.patch | 5.16 KB | fabian.marz |
#27 | interdiff-2786193-24-27.txt | 867 bytes | fabian.marz |
Comments
Comment #2
tedbowGiven there shouldn't be 2 links that say "Quick Edit" but when click the first "Quick Edit" link it opens the offcanvas tray(Outside In) AND trigger the Quick Module functionality. This seems very confusing. I don't think just removing the link is going to solve the problem.
It seems we need the 2 links but need to solve the naming issue.
Comment #3
larowlanI think this is a duplicate of #1956134: Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)
Is the 'adopting D7 usability to D6' tag correct?
There are two entities here - the block (config/placement) and block_content (content entity).
Comment #4
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #5
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI am tempting to mark this duplicate, and continue in #1956134: Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)
Comment #6
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
Let's keep this one open but keep the scope to just removing the duplicate link, which to the user is a bug.
The other issue can be where we handle the broader design pattern.
Comment #7
Bojhan CreditAttribution: Bojhan as a volunteer and commentedBut @tedblow clearly points out it won't solve the problem? I am inclined to agree.
Comment #8
tkoleary CreditAttribution: tkoleary at Acquia commented@bojhan
I think you're right.
Until we have a better solution (eg. when this is in core and we can unify the two) let's change the text to "Show Settings".
Comment #9
tedbowI think the difference between "Show Settings" and "Configure Block" would be. Not obivous which one shows in the "setting tray"
Ok. Here is a try at the solution
For Content blocks the "Setting tray" link is changed to "Quick edit settings"
I don't think there are any great options here.
Another option would be
Change both links on Content Blocks to include :
Quick edit content
Quick edit settings
Comment #10
tedbowChanging the title because we can't remove a link.
This was a bug and has been fixed.
Comment #11
larowlanWorks for me, but needs a test right?
Comment #12
tedbow@larowlan yep you are right.
Here is a test to make sure the correct link is changed to "Quick edit settings"
Comment #13
larowlanLooking good, couple of things
Why self:: and not $this->?
There may already be a trait for this. If not, we should make one.
Comment #14
tedbow1. Using "self::" because it is static function.
2. It might be a good idea to make a trait but in this experimental module so we can keep making changes to 8.2.x branch. But we can't make changes out site of the module in 8.2.x
Comment #16
tedbowNot sure why DrupalCI link starts with "/checkout/".
Changed to use strstr to look inside $href string, not for equality.
Comment #17
tim.plunkettI don't think appending this string is safe for translations. It should redeclare the string within $this->t().
Yes, it is static in PHPUnit. But we still call it with $this-> because a) you can b) it is more consistent
Testbot installs into a subdirectory to catch bugs and assumptions around similar things.
Comment #18
tedbow@tim.plunkett thanks for feedback
1. Yep don't know what I was thinking :(. Fixed!
2. changed to use $this->
Comment #19
tim.plunkettA follow-up to make these a trait would be nice, I think we redo this about 9 other places already
Looks good to me!
Comment #21
tedbowUghh just a re-roll
Comment #22
tim.plunkett+1
Comment #23
20th CreditAttribution: 20th commentedThis patch is not tested yet. It does not pass its tests because right now
OutsideInBlockFormTest::testBlocks
tests are disabled in #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly:Status here should be changed to Postponed until blocker issue is fixed and tests can be executed again.
Comment #24
tedbow@20th thanks for pointing that out.
I have the testing of this functionality into it's own test method. It is easier to read anyways if it is in it's own test method. The only reason I put it in the other is to save test time by not having to install drupal again. I am not sure about the balance between simpler methods and short test suite runs.
I added a todo about removing assertWaitOnAjaxRequest() call if this is not need after #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly
We could have a todo about moving the code back into testBlocks() but maybe it is better on it's own.
Comment #25
Bojhan CreditAttribution: Bojhan as a volunteer and commentedLooks great, the word settings clearly annotates its meaning.
Comment #27
fabian.marz CreditAttribution: fabian.marz commentedWhat about this earlier consideration? Why did you chose changing the link in patch 24 just for the offcanvas and not for the content?
I rerolled patch 24. Regarding to #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly I removed the obsolete Ajax request function call.
Comment #28
tedbow@fabian.marz thanks for re-rolling.
I think wording as it is not simpler and we got UX sign off from @Bohjan in #25.
Comment #30
tedbowRe-roll
Comment #31
Wim LeersBack to RTBC. Manually confirmed that #30 is functionally equivalent with #21, which @tim.plunkett RTBC'd.
Comment #32
lauriiiWas there a particular reason to use FALSE to state no value? Wouldn't null make sense?
Comment #33
tedbow@lauriii FALSE what is other testing functions in creating blocks:
\Drupal\block_content\Tests\BlockContentTestBase::createBlockContent
\Drupal\block_content\Tests\BlockContentTranslationUITest::createBlockContent
\Drupal\Tests\block_content\Functional\BlockContentTestBase::createBlockContent
They share the signature
protected function createBlockContent($title = FALSE, $bundle = 'basic', $save = TRUE) {
well least the first 2 arguments are the same in all 3
Comment #34
alexpottThis really ought to be randomName() not randomMachineName().
Re #32 I agree it's odd but the way it is currently coded NULL, FALSE and empty string will all result in a random name. So I wouldn't hold up the patch on this method's signature.
Comment #35
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #36
tedbow@gaurav.kapoor thanks for fixing #34
I think back to RTBC because @alexpott said he was going hold up the patch for #32
@alexpott & @larowlan thanks for addressing the issue!
Comment #37
lauriiiThank you @alexpott and @tedbow for addressing #32. Other than that this looks good for me. Also, the wording has been accepted by @Bojhan on #25.
Committed b49d243 and pushed to 8.4.x. Thanks!
Comment #39
tim.plunkettLooks like d.o ate the metadata change
Comment #40
star-szrYup, let's commit to 8.3.x as well since it's an experimental module :)
Comment #42
tedbowOk. DrupalCI keeps trying to run #35 against 8.4.x when set to RTBC. Not sure how to solve this.
Comment #44
lauriiiCherry picked ed69c34 and pushed to 8.3.x.
Comment #45
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedLOL
Comment #47
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)