Problem/Motivation

When you add a field block, there are two fields for title.
One from the block system 'display title' and another from the field API (formatter title).
The default for display title is true.
I don't think we should default to true, and I'm not sure we even need it given we have the existing label from field UI.
It is confusing, but also you can end up with two titles

Proposed resolution

Process callback attached to the configure form that removes the 'display title' checkbox and defaults it to FALSE?
Decide on approach
Fix

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#20 Screen Shot 2018-02-06 at 7.56.50 AM.png198.93 KBKristen Pol
#16 Screen Shot 2018-01-31 at 8.20.07 AM.png238.41 KBKristen Pol
#16 Screen Shot 2018-01-31 at 8.17.49 AM.png216.47 KBKristen Pol
#15 2936085-field_block-15.patch599 bytestim.plunkett
#12 2936085-12.patch1.08 KBchiranjeeb2410
#11 interdiff-2936085-3-4.txt1.08 KBchiranjeeb2410
#5 Screen Shot 2018-01-15 at 11.04.44 PM.png209.61 KBKristen Pol
#4 Screen Shot 2018-01-15 at 10.54.26 PM.png209.13 KBKristen Pol
#3 2936085-field_block-3-uncheck.patch599 bytestim.plunkett
#3 2936085-field_block-3-hide.patch716 bytestim.plunkett
Screenshot-2018-1-12 Edit layout for Layouts eg drupal 8 5 x(1).png2.61 KBlarowlan
Screenshot-2018-1-12 Edit layout for Layouts eg drupal 8 5 x.png10.26 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue summary: View changes
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2936085-field_block-3-hide.patch716 bytes
2936085-field_block-3-uncheck.patch599 bytes

Here's one of each.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
FileSize
Screen Shot 2018-01-15 at 10.54.26 PM.png209.13 KB

I'm not sure I'm testing this properly. I tested the first patch and used the builder to place body content on a node. I don't see a checkbox for the title but if I show the title, I have two titles showing. If I visually hide the title, then I don't see either of the titles. I was expecting to only see one title but I'm flying blind since there are no steps to reproduce.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue summary: View changes
FileSize
Screen Shot 2018-01-15 at 11.04.44 PM.png209.61 KB

Assuming I'm testing the right thing, I checked the 2nd patch and I see the checkbox and it's unchecked by default. The results for this patch look right to me. I was able to create 3 types:

* 2 titles (because I checked the checkbox)
* 1 title (because I didn't check the checkbox and kept label as visible)
* no title (because I didn't check the checkbox and marked label as hidden)

Code looks fine in patch 2. I vote for that one. Patch is this one 2936085-field_block-3-uncheck.patch. I would mark RTBC if only that patch was active.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue summary: View changes
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Marking back as needs work only to get it down to one patch.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Yes +1 for patch 2.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Parent issue: » #2940212: [meta] Miscellaneous UI issues for the Layout Builder module
chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
FileSize
interdiff-2936085-3-4.txt1.08 KB

@Kristen Pol, attached interdiff for patches on #3. Would be uploading fresh patch containing changes from both patches.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2936085-12.patch1.08 KB

@Kristen Pol,

Uploaded fresh patch containing changes included in both patches for comment #3

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Needs work

Those two patches are opposite approaches, it doesn't make sense to combine them.

chiranjeeb2410’s picture

chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented

@tim.plunkett, we could finalise on the 2936085-field_block-3-uncheck.patch as it seems to work fine
as addressed in comment #5

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2936085-field_block-15.patch599 bytes

I agree.
Reuploading my original patch.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
FileSize
Screen Shot 2018-01-31 at 8.17.49 AM.png216.47 KB
Screen Shot 2018-01-31 at 8.20.07 AM.png238.41 KB

Thanks. I tested the patch from #15. Everything works as expected for the checkbox behavior and the patch is fine. Side note: changing the title text does not work but that would be a different issue and I imagine it's already reported somewhere. I'll try to find the issue.

I tested/observed the following:

  • Initially, the body block was already there with no label or title
  • These combinations were tried: title but hidden label, both title and label shown, title hidden and label shown inline

    • RTBC'ing this.

    Kristen Pol’s picture

    Kristen Pol
    she/her
    Primary language English
    Location Santa Cruz, CA, USA
    CreditAttribution: Kristen Pol at Hook 42 commented
    Status: Needs review » Reviewed & tested by the community
    larowlan’s picture

    larowlan
    Location 🇦🇺🏝.au GMT+10
    CreditAttribution: larowlan at PreviousNext commented
    Status: Reviewed & tested by the community » Needs review

    Side note: changing the title text does not work but that would be a different issue and I imagine it's already reported somewhere. I'll try to find the issue.

    Would be good to verify that we have an issue for that before we close this one down.

    Kristen Pol’s picture

    Kristen Pol
    she/her
    Primary language English
    Location Santa Cruz, CA, USA
    CreditAttribution: Kristen Pol at Hook 42 commented

    I wonder if this is related: #1926736: Allow block titles to be automatically tied to the underlying object

    I didn't see anything else obvious in the queue.

    Kristen Pol’s picture

    Kristen Pol
    she/her
    Primary language English
    Location Santa Cruz, CA, USA
    CreditAttribution: Kristen Pol at Hook 42 commented
    FileSize
    Screen Shot 2018-02-06 at 7.56.50 AM.png198.93 KB

    Sorry for the lack of clarity on the title issue mentioned above. Steps to reproduce:

    • Add a block
    • Change the title of the block in the form
    • Save the changes
    • Title does not show up changed in the layout builder

    tim.plunkett’s picture

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

    #1926736: Allow block titles to be automatically tied to the underlying object is slightly different in the opposite direction.

    I just opened #2942623: Configured block labels are sometimes overridden by the render array to address the problem @Kristen Pol described.
    Setting this back to RTBC

    xjm’s picture

    xjm
    she/her
    Primary language English
    CreditAttribution: xjm at Acquia commented

    Thanks @chiranjeeb2410 for working on this! In this case the approach changes made were not actually correct (as @tim.plunkett described) so I've unset the issue credit for this particular issue.

    xjm’s picture

    xjm
    she/her
    Primary language English
    CreditAttribution: xjm at Acquia commented
    Version: 8.6.x-dev » 8.5.x-dev

    So a big +1 for this issue; this was also something I noticed in my prior review of LB patches.

    Committed and pushed to 8.6.x! This is also fairly non-disruptive, especially for a beta module, and it's a good usability improvement. So, setting RTBC against 8.5.x for backport following our beta commit freeze.

    • xjm committed 6d06905 on 8.6.x 
      Issue #2936085 by tim.plunkett, Kristen Pol, larowlan: Remove 'display...

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 15: 2936085-field_block-15.patch, failed testing. View results

    Kristen Pol’s picture

    Kristen Pol
    she/her
    Primary language English
    Location Santa Cruz, CA, USA
    CreditAttribution: Kristen Pol at Hook 42 commented
    Issue tags: +Needs reroll
    chiranjeeb2410’s picture

    chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented
    Version: 8.5.x-dev » 8.6.x-dev
    tim.plunkett’s picture

    tim.plunkett
    he/him
    Primary language English
    Location Philadelphia
    CreditAttribution: tim.plunkett at Acquia commented
    Version: 8.6.x-dev » 8.5.x-dev
    Status: Needs work » Reviewed & tested by the community
    Issue tags: -Needs reroll

    This was committed to 8.6.x, and it cleanly applies to 8.5.x
    It just cannot apply to 8.6.x anymore because it's already in the code base.

    Kristen Pol’s picture

    Kristen Pol
    she/her
    Primary language English
    Location Santa Cruz, CA, USA
    CreditAttribution: Kristen Pol at Hook 42 commented

    @tim.plunkett Ah! Thanks for the explanation.

    • xjm committed e751f4d on 8.5.x 
      Issue #2936085 by tim.plunkett, Kristen Pol, larowlan: Remove 'display...
    xjm’s picture

    xjm
    she/her
    Primary language English
    CreditAttribution: xjm at Acquia commented
    Status: Reviewed & tested by the community » Fixed

    Backported to 8.5.x as well now. Thanks!

    tim.plunkett’s picture

    tim.plunkett
    he/him
    Primary language English
    Location Philadelphia
    CreditAttribution: tim.plunkett at Acquia commented
    Component: layout.module » layout_builder.module

    Status: Fixed » Closed (fixed)

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