Differentiate 2 "Quick edit" links for custom blocks

Given 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.

I 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).

I 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.)

But @tedblow clearly points out it won't solve the problem? I am inclined to agree.

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".

I 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

Changing the title because we can't remove a link.

Remove the second quick edit link since the first already opens the tray and triggers quick edit of the text.

This was a bug and has been fixed.

Works for me, but needs a test right?

@larowlan yep you are right.

Here is a test to make sure the correct link is changed to "Quick edit settings"

Looking good, couple of things

1. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -116,6 +125,20 @@ public function testBlocks() {
   +    self::assertEquals('', $href);
   ...
   +    self::assertEquals('/admin/structure/block/manage/custom/offcanvas?destination=user/2', $href);
   

   Why self:: and not $this->?

2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -155,4 +178,60 @@ protected function openBlockForm($block_selector) {
   +  protected function createBlockContent($title = FALSE, $bundle = 'basic', $save = TRUE) {
   ...
   +  protected function createBlockContentType($label, $create_body = FALSE) {
   

   There may already be a trait for this. If not, we should make one.

1. 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

--- Expected
+++ Actual
@@ @@
-'/admin/structure/block/manage/custom/offcanvas?destination=user/2'
+'/checkout/admin/structure/block/manage/custom/offcanvas?destination=user/2'

Not sure why DrupalCI link starts with "/checkout/".

Changed to use strstr to look inside $href string, not for equality.

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -39,6 +39,10 @@ function outside_in_contextual_links_view_alter(&$element, $items) {
   +      $element['#links']['outside-inblock-configure']['title'] .= ' settings';
   

   I don't think appending this string is safe for translations. It should redeclare the string within $this->t().

2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -116,6 +125,20 @@ public function testBlocks() {
   +    self::assertEquals('', $href);
   

   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.

@tim.plunkett thanks for feedback

1. Yep don't know what I was thinking :(. Fixed!
2. changed to use $this->

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -155,4 +178,60 @@ protected function openBlockForm($block_selector) {
+  protected function createBlockContent($title = FALSE, $bundle = 'basic', $save = TRUE) {
...
+  protected function createBlockContentType($label, $create_body = FALSE) {

A follow-up to make these a trait would be nice, I think we redo this about 9 other places already

Looks good to me!

Ughh just a re-roll

+1

This 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:

  /**
   * Tests opening Offcanvas tray by click blocks and elements in the blocks.
   */
  public function testBlocks() {
    // @todo: re-enable once https://www.drupal.org/node/2830485 is resolved.
    $this->markTestSkipped('Test skipped due to random failures in DrupalCI, see https://www.drupal.org/node/2830485');

    $web_assert = $this->assertSession();

Status here should be changed to Postponed until blocker issue is fixed and tests can be executed again.

@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.

Looks great, the word settings clearly annotates its meaning.

What about this earlier consideration? Why did you chose changing the link in patch 24 just for the offcanvas and not for the content?

Another option would be
Change both links on Content Blocks to include :
Quick edit content
Quick edit settings

I rerolled patch 24. Regarding to #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly I removed the obsolete Ajax request function call.

@fabian.marz thanks for re-rolling.

I think wording as it is not simpler and we got UX sign off from @Bohjan in #25.

Re-roll

Back to RTBC. Manually confirmed that #30 is functionally equivalent with #21, which @tim.plunkett RTBC'd.

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -364,4 +372,80 @@ protected function pressToolbarEditButton() {
+  protected function createBlockContent($title = FALSE, $bundle = 'basic', $save = TRUE) {

Was there a particular reason to use FALSE to state no value? Wouldn't null make sense?

@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

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -364,4 +372,80 @@ protected function pressToolbarEditButton() {
+    $title = $title ?: $this->randomMachineName();

This 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.

@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!

Thank 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!

Looks like d.o ate the metadata change

Yup, let's commit to 8.3.x as well since it's an experimental module :)

Ok. DrupalCI keeps trying to run #35 against 8.4.x when set to RTBC. Not sure how to solve this.

Cherry picked ed69c34 and pushed to 8.3.x.

LOL

Changing to new settings_tray.module component. @drpal thanks for script help! :)