Problem/Motivation

In #2781575: Determine ideal field order and visibility for "quick edit" blocks in off canvas tray and UX meeting it was determined that the "Title" field on the block for is confusing. Often times the block title is not displayed but the user can still change it.

Also "title" is confusing because is it the title of the site(when used on branding block) or the title just for the block.

Here is a screen of the unpatched module. This shows a block with the block title not displayed.

You can see title input box is shown even though this will have not effect unless the user also checks "Display title"
Also "Title" and "Display Title" are not clear this is for the specific block.

Proposed resolution

Hide the title field if "Display Title" is not checked.

Change label to "Block Title" and "Display Block Title"
Here is screenshot the same block as above but with patched module

You can see that when the title is not displayed on the block then the title input is not shown.
The label for the checkbox is also changed to "Display Block Title"

Once you check the checkbox

You see the title input show up below the checkbox.
The input label is also changed to "Block Title"

Here is short video showing a unpatched and patched version https://youtu.be/hj5Rf2kxb24

Remaining tasks

Create patch
Add tests
Create screenshots
Create demo video
UX Review

User interface changes

See "Proposed resolution" above

API changes

None

Data model changes

None

CommentFileSizeAuthor
#14 patched_title_checked.png19.42 KBtedbow
#14 patched_no_title.png17.68 KBtedbow
#14 title_current.png19.08 KBtedbow
#10 2882729-10.patch4.71 KBtedbow
#10 interdiff-2882729-9-10.txt5.57 KBtedbow
#9 2882729-9.patch6.42 KBtedbow
#9 interdiff-2882729-7-9.txt2.65 KBtedbow
#7 2882729-7.patch7.56 KBtedbow
#7 interdiff-2882729-3-7.txt7.62 KBtedbow
#3 2882729-3.patch8.2 KBtedbow
#3 interdiff-2882729-2-3.txt6.76 KBtedbow
#2 2882729-2.patch8.87 KBtedbow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Active » Needs review
FileSize
2882729-2.patch8.87 KB

Here are title related changes from #2781575: Determine ideal field order and visibility for "quick edit" blocks in off canvas tray

Also added comprehensive tests. The tests showed some need for changes to \Drupal\outside_in\Block\BlockEntityOffCanvasForm::processLabelInput()

To see what could go wrong look at \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testHiddenFormElements()

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-2882729-2-3.txt6.76 KB
2882729-3.patch8.2 KB

Cleans up test class, I had commented out other test functions.

Changes label of input from "Title" to "Block Title"

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Title: In Off Canvas block form hide Title unless it will be displayed and Block Title » In off-canvas block form hide Title input unless it will be displayed and change label to Block Title
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Assigned: tedbow » Unassigned
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented 
  1. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -52,10 +52,43 @@ public function form(array $form, FormStateInterface $form_state) {
+    $form['settings']['label_display']['#weight'] = -100;
+    $form['settings']['label']['#states']['visible'] = [

    Technically this whole thing should be wrapped in an if (isset($form['settings']['label_display']) && isset($form['settings']['label'])) { since it could theoretically not exist. 

  2. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -52,10 +52,43 @@ public function form(array $form, FormStateInterface $form_state) {
+    $form['settings']['label']['#process'][] = [get_class($this), 'processLabelInput'];

    get_class($this) can now be replaced by static::class 

  3. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -52,10 +52,43 @@ public function form(array $form, FormStateInterface $form_state) {
+    $input = $form_state->getUserInput();

    Is there a reason getUserInput is used instead of getValues? 

  4. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -52,10 +52,43 @@ public function form(array $form, FormStateInterface $form_state) {
+      // Set element value because this is require field.

    required 

  5. +++ b/core/modules/outside_in/src/Block/BlockEntityOffCanvasForm.php
@@ -52,10 +52,43 @@ public function form(array $form, FormStateInterface $form_state) {
+      $values = $form_state->getValues();
+      $values['settings']['label'] = $element['#value'];
+      $form_state->setValues($values);

    This can be $form_state->setValue(['settings', 'label'], $element['#value']); 

  6. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -469,4 +471,87 @@ public function getBlockSelector(Block $block) {
+      /* ** Check that Title field is not marked as required if hidden. **  */
...
+      /* ** Check Title is retained when previously hidden. **  */
...
+      /* ** Check Title can be updated when not hidden. **  */

    These should all be // Inline comments. 

  7. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -469,4 +471,87 @@ public function getBlockSelector(Block $block) {
+
+
...
+    }
+
+  }

    Extra new line 

  8. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -469,4 +471,87 @@ public function getBlockSelector(Block $block) {
+   * Determine if the label input is visible.

    Determines 

  9. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -469,4 +471,87 @@ public function getBlockSelector(Block $block) {
+  protected function labelInputIsVisible() {

    isLabelInputVisible? 

  10. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInJavascriptTestBase.php
@@ -42,7 +42,7 @@ public function enableTheme($theme) {
-    $web_assert->waitForElementVisible('css', '#drupal-off-canvas');
+    $this->assertNotEmpty($web_assert->waitForElementVisible('css', '#drupal-off-canvas'));

    I don't understand this change

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-2882729-3-7.txt7.62 KB
2882729-7.patch7.56 KB

@tim.plunkett thanks for the review.

1. fixed
2. fixed
3. Yes added comment in code

// Use getUserInput() instead of getValues() because 'label_display' will
// not be available from getValues() at this point.

I confirm this again by step debugging and changing to getValues() makes the test fail.

4. fixed
5. fixed
6. Fixed. was trying to make these comments stand out because they are major steps in the test but hopefully empty lines between the sections will be enough.
7. Think I found the line you were talking about and removed.
8. fixed
9. yes, thats better, Fixed
10. During making the changs I was getting weird test failures. It was because originally when wrote this test I thought that
$web_assert->waitForElementVisible('css', '#drupal-off-canvas')
Would actually cause a failure if the element was not visible after the wait. This was my mistake it just returns back NULL.
I just tested and it can be removed and test still pass.
So basically this just waits for the tray to open but doesn't fail if it doesn't open. Just things later that depend on it opening will fail.

So removed it. Will file follow up.

FOLLOW UP: #2883483: Assert that calls to waitForElementVisible() actually return element in OutsideIn javascript tests.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

Looks like the reason I need to use getInput() as @tim.plunkett asked in #6.3. was because my local site had some problem.

I reinstalled and I don't need to and probably don't need #process at all.

Will update patch which should make it much simpler.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-2882729-7-9.txt2.65 KB
2882729-9.patch6.42 KB

Ok. so this removes the process callback for the label element. So now it is simply use the #states API.

I left the test in. The only thing I changed about the test is the part where checks that you can set the label to "" and then hide it. That will not work without the process callback but that seems a bit of a forced example.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
FileSize
interdiff-2882729-9-10.txt5.57 KB
2882729-10.patch4.71 KB

This removes the test testTitleInput(). We don't need this anymore because we are not doing anything special for processing of the title input.

We were already testing a block, system_powered_by_block, where label was hidden and then show it. So moved the checkbox test to there.

Also added to assertOffCanvasBlockFormIsValid() checking the labels are changed and the Block title input is not shown if checkbox is not checked.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks great! Thanks @tedbow

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Needs screenshots, +Usability

This could use UX review, and screenshots to make that review easier.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Status: Reviewed & tested by the community » Needs review

NR as per #12

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Issue summary: View changes
FileSize
title_current.png19.08 KB
patched_no_title.png17.68 KB
patched_title_checked.png19.42 KB

Ok added screenshots and video to summary

Bojhan’s picture

Bojhan CreditAttribution: Bojhan as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, this leverages how we intent to use the pattern for hiding fields when they are not necessary to see.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Makes a lot of sense. Does what it says on the tin, and comes with tests. Win!

Committed and pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • webchick committed 5389942 on 8.4.x 
    Issue #2882729 by tedbow, tim.plunkett, Bojhan: In off-canvas block form...

  • webchick committed 85232c7 on 8.3.x 
    Issue #2882729 by tedbow, tim.plunkett, Bojhan: In off-canvas block form...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Component: outside_in.module » settings_tray.module

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