Problem/Motivation

There's some misusage of BEM in the layout builder markup. The instances I could find so far:

  • layout-builder--layout__region class is added to element that doesn't have layout-builder class.
  • layout-builder--layout class is added to element that doesn't have layout-builder class.

Proposed resolution

Update the Layout Builder UI classes to follow BEM standards.

Remaining tasks

Requires manual testing in Bartik and Umami due to some styles having less specificity.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

CSS classes used in Layout Builder's user interface have been renamed in accordance with BEM standards (see http://getbem.com). For example, classes like `.layout-section` are now `.layout-builder__section`.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

tim.plunkett’s picture

Component: other » layout_builder.module
Issue tags: -Layout Builder +Blocks-Layouts
tim.plunkett’s picture

Status: Active » Postponed (maintainer needs more info)

Please clarify why this is a stable blocker.

lauriii’s picture

The current markup is highly confusing for frontend developers. It is also against our frontend core gates. When Layout Builder gets a stable release, we have to lock down the markup for BC (in Stable), and there's no longer a good way of fixing this. Given those parameters, I suggest we solve this before Layout Builder gets released but of course the final decision should be made by a release manager.

tim.plunkett’s picture

Status: Postponed (maintainer needs more info) » Active

Those are not in the templates.
They are added programmatically added for usage by the Layout Builder UI itself.

dead_arm’s picture

Title: Make layout builder properly implement BEM » Update the Layout Builder UI classes to implement BEM naming conventions
Issue summary: View changes
Status: Active » Needs review
FileSize
7.98 KB

In order to apply BEM class names, I added layout-builder as a class to the parent. Where layout was used as the block of the class previously, I updated it with layout-builder.

The Add Section and Add Block links used the classes new-section and new-block, and I've updated the classes to also use add instead of new in order to simplify naming. These links shared styling and it made sense for them to be combined into a single class layout-builder__link--add.

lauriii’s picture

Thank you @dead_arm! The patch looks awesome 💎.

Any thoughts on converting layout-builder-block as well into layout-builder__block?

tim.plunkett’s picture

@dead_arm and I discussed layout-builder-block. The difference there is that all of the above classes (including the root layout-builder class) are only present in the Layout Builder UI, while editing the layout.

However, layout-builder-block is used always, even during runtime.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

That makes sense. In that case, I think this is ready. There are no more class names that should be updated. I also grepped the removed class names to make sure they are not being used anymore. Thank you both!

xjm’s picture

Issue tags: +Layout Builder frontend issue

Another signoff/+1 from someone frontend-y would be good here so that @lauriii can commit this. :)

xjm’s picture

Also this says it should be manually tested with both Bartik and Umami; did that happen?

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Yep let's do that too.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
110.25 KB
118.26 KB
  • Installed Standard at the tip of 8.7.x (3eeddcf)
  • Installed LB and enabled it for the default view of the Article node type, with overrides allowed.
  • Created a new article (an homage to the classic not-work-safe Onion article "Man Walks on &#^!?*#ing Moon", which is why I'm not embedding them here)
  • Went to edit the layout and took a screenshot
  • Applied the patch and cleared caches (drush cr)
  • Added a couple of sections, moved some blocks around, and removed sections.

Everything behaved as expected (screenshot attached, after moving things around). It worked perfectly and looked perfect.

This is ready.

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Ah, forgot to test in Umami. Whoops! Back to NR for that.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
372.59 KB
387.99 KB
  • Installed Umami at the tip of 8.7.x (3eeddcf)
  • Installed LB and enabled it for the default and full content displays of the Recipe content type, with overrides enabled for the default display.
  • Edited Thai Green Curry, added a section, moved some stuff around, changed a block configuration, added a block, removed a block. Took a screenshot.
  • Applied the patch and cleared caches (drush cr).
  • Refreshed the page and made more modifications. Took another screenshot.

Before and after the patch, the LB interface worked great! I saw no obvious inconsistencies or breaks. I now really think this is ready to go.

DyanneNova’s picture

The code looks good to me too. I think this is ready to go!

xjm’s picture

Assigned: Unassigned » phenaproxima
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record, +Needs release note

Thanks @DyanneNova!

Needs a CR and release note snippet which Adam is going to add. Thanks!

phenaproxima’s picture

Issue tags: -Needs change record
phenaproxima’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs release note

Release note added to the IS.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

phenaproxima’s picture

Assigned: phenaproxima » Unassigned

Unassigning.

  • xjm committed 1a945e7 on 8.7.x
    Issue #3034347 by dead_arm, phenaproxima, tim.plunkett, lauriii: Update...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.7.0 release notes

Tim walked me through the patch here. Things we looked at:

  • The specifics of the BEM standard, and whether all the renamed things followed the standard. The key thing to this patch is adding the parent .layout-builder class so that BEM can be adopted nicely for the rest of the CSS. I then manually did a git diff --color-words --staged to check each replacement name. ✅
     

  • Whether we updated all usages of the renamed things. @lauriii confirmed this in #9. ✅
     

  • Whether we renamed all the things we should:

    • @tim.plunkett pointed me to #7 and #8 for the primary exception for .layout-builder-block.
    • I also asked about things like .ui-sortable-helper and whether those come from something other than LB -- per @tim.plunkett that's just jQuery UI stuff.
    • I asked about whether classes coming from JavaScript should be taken into consideration. Those are prefixed with .js- (which is apparently the standard) and left out of scope here. The only change in JS is where it does stuff around .layout-builder__region, already defined and renamed in the stylesheet itself.
    • I opened and read the whole stylesheet with the patch applied. The remainder of the stylesheet is offcanvas stuff, which we shouldn't touch, because the whole problem of offcanvas CSS leaking into the theme or vice versa is a thing and we should leave that well alone. Within what's there, the inline block stuff already follows the standard correctly, as does .layout-selection itself (because it is the top-level parent).


     

  • Finally, in terms of checking for visual regressions. @tim.plunkett pointed out that the things to pay careful attention to are rules where the specificity has changed, because when they become less specific it might allow a different rule somewhere to clobber them. We have a couple rules that go from two elements to one in the patch, and one rule that goes from three elements to one. I specifically checked each of the affected elements in the screenshots. ✅

    Relatedly, this is amazing: https://specifishity.com/

Glad we caught this before stable! Thanks @lauriii and @dead_arm. Committed to 8.7.x and published the CR.

Status: Fixed » Closed (fixed)

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