Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 havelayout-builder
class.layout-builder--layout
class is added to element that doesn't havelayout-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`.
Comment | File | Size | Author |
---|---|---|---|
#15 | 3034347-15-after.png | 387.99 KB | phenaproxima |
#15 | 3034347-15-before.png | 372.59 KB | phenaproxima |
#13 | 3034347-13-after.png | 118.26 KB | phenaproxima |
#13 | 3034347-13-before.png | 110.25 KB | phenaproxima |
#6 | 3034347-bem-6.patch | 7.98 KB | dead_arm |
Comments
Comment #2
tim.plunkettComment #3
tim.plunkettPlease clarify why this is a stable blocker.
Comment #4
lauriiiThe 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.
Comment #5
tim.plunkettThose are not in the templates.
They are added programmatically added for usage by the Layout Builder UI itself.
Comment #6
dead_armIn order to apply BEM class names, I added
layout-builder
as a class to the parent. Wherelayout
was used as the block of the class previously, I updated it withlayout-builder
.The Add Section and Add Block links used the classes
new-section
andnew-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.Comment #7
lauriiiThank you @dead_arm! The patch looks awesome 💎.
Any thoughts on converting
layout-builder-block
as well intolayout-builder__block
?Comment #8
tim.plunkett@dead_arm and I discussed
layout-builder-block
. The difference there is that all of the above classes (including the rootlayout-builder
class) are only present in the Layout Builder UI, while editing the layout.However,
layout-builder-block
is used always, even during runtime.Comment #9
lauriiiThat 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!
Comment #10
xjmAnother signoff/+1 from someone frontend-y would be good here so that @lauriii can commit this. :)
Comment #11
xjmAlso this says it should be manually tested with both Bartik and Umami; did that happen?
Comment #12
xjmYep let's do that too.
Comment #13
phenaproximaEverything behaved as expected (screenshot attached, after moving things around). It worked perfectly and looked perfect.
This is ready.
Comment #14
phenaproximaAh, forgot to test in Umami. Whoops! Back to NR for that.
Comment #15
phenaproximaBefore 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.
Comment #16
DyanneNovaThe code looks good to me too. I think this is ready to go!
Comment #17
xjmThanks @DyanneNova!
Needs a CR and release note snippet which Adam is going to add. Thanks!
Comment #18
phenaproximaChange record written: https://www.drupal.org/node/3035954
Comment #19
phenaproximaRelease note added to the IS.
Comment #20
tim.plunkettGreat, thanks!
Comment #21
phenaproximaUnassigning.
Comment #23
xjmTim 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 agit 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:
.layout-builder-block
..ui-sortable-helper
and whether those come from something other than LB -- per @tim.plunkett that's just jQuery UI stuff..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..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.