Fixes less compilation problems in the Magento/blank theme #24001

hostep · 2019-08-03T09:21:39Z

Description (*)

Hi

This PR is fixing less compilation problems in Magento's Blank theme as being reported in #23619
A PR for fixing similar problems in the Luma theme exists over here.

These less compilation problems can be found when using the original less.js compiler. Using version 2 or 3 of this compiler will output these problems.
These problems aren't being shown when using the php port of this compiler which Magento uses by default.

But the fact that these compilation errors are being reported means that certain less code in Magento isn't quite right.
And by digging in further, I was able to fix some problems which even improve the rendering on the frontend for features which were supposed to work, but didn't work due to these (hidden) errors.

This PR consists of 5 commits.

Commit 1

The first commit is not directly related to the Blank theme, but caused a compilation problem with the Blank theme nonetheless:

The selector .abs-modal-overlay is being defined in the backend theme, but used in the lib code, so that's incorrect. Moved the usage to the backend theme. The resulting css files don't change by doing this.

Commit 2

The second commit is simply cleanup of code causing most of the compilation problems:

The selector .abs-no-display-desktop was being used in a different media query then where it was defined, resulting in the compiler not being able to find it. By moving it in the correct media query, we fix the compilation problem.
The selector .abs-clearfix is being defined in the backend theme, but was being used in a frontend theme, so this never worked, removed its usage on the frontend in this case.

After this second commit, the resulting CSS is almost unchanged, except for a single change which cuts down the filesize of the resulting css file a tiny bit while do keeping the css code the same in behavior.
This is the diff of the css files after the second commit:

$ diff -ur before/static/frontend/ after/pub/static/frontend/
diff -ur before/static/frontend/Magento/blank/en_US/css/styles-l.css after/pub/static/frontend/Magento/blank/en_US/css/styles-l.css
--- before/static/frontend/Magento/blank/en_US/css/styles-l.css 2019-08-01 20:53:24.000000000 +0200
+++ after/pub/static/frontend/Magento/blank/en_US/css/styles-l.css  2019-08-01 21:03:10.000000000 +0200
@@ -248,6 +248,7 @@
     margin-bottom: 0;
   }
   .abs-no-display-desktop,
+  .opc-estimated-wrapper,
   .sidebar .block.widget .pager .item:not(.pages-item-next):not(.pages-item-previous) {
     display: none;
   }
@@ -1313,9 +1314,6 @@
     float: right;
     margin: 22px 0 0;
   }
-  .opc-estimated-wrapper {
-    display: none;
-  }
   .opc-progress-bar-item {
     width: 185px;
   }

Commit 3

The third commit should in theory improve the rendering of a certain component in Magento Commerce.
Unfortunately I don't have a copy over here at the moment of Magento Commerce to test this out.
So if somebody could test this out in the Magento_AdvancedCheckout module on the frontend to see how and what this fixes exactly, that would be great!

it should probably apply the mixin .lib-clearfix to that .block-addbysku .block-content selector in this module
it should probably apply the mixin @abs-blocks-2columns to that .block-addbysku .block-content .box selector in this module
After this fix it should actually apply these mixins, before this fix these probably weren't being applied.

The diff in the generated css file after the third commit is a little bit more involved:

$ diff -ur before/pub/static/frontend/ after/pub/static/frontend/
diff -ur before/pub/static/frontend/Magento/blank/en_US/css/styles-l.css after/pub/static/frontend/Magento/blank/en_US/css/styles-l.css
--- before/pub/static/frontend/Magento/blank/en_US/css/styles-l.css 2019-08-01 20:53:24.000000000 +0200
+++ after/pub/static/frontend/Magento/blank/en_US/css/styles-l.css  2019-08-01 21:14:41.000000000 +0200
@@ -56,6 +56,7 @@
     width: auto;
   }
   .abs-blocks-2columns,
+  .column .block-addbysku .block-content .box,
   .login-container .block,
   .account .column.main .block:not(.widget) .block-content .box,
   .magento-rma-guest-returns .column.main .block:not(.widget) .block-content .box,
@@ -63,6 +64,7 @@
   .sales-guest-view .column.main .block:not(.widget) .block-content .box {
     width: 48.8%;
   }
+  .column .block-addbysku .block-content .box:nth-child(odd),
   .login-container .block:nth-child(odd),
   .account .column.main .block:not(.widget) .block-content .box:nth-child(odd),
   .magento-rma-guest-returns .column.main .block:not(.widget) .block-content .box:nth-child(odd),
@@ -71,6 +73,7 @@
     clear: left;
     float: left;
   }
+  .column .block-addbysku .block-content .box:nth-child(even),
   .login-container .block:nth-child(even),
   .account .column.main .block:not(.widget) .block-content .box:nth-child(even),
   .magento-rma-guest-returns .column.main .block:not(.widget) .block-content .box:nth-child(even),
@@ -134,6 +137,8 @@
   .abs-pager-toolbar:after,
   .block-cart-failed .block-content:before,
   .block-cart-failed .block-content:after,
+  .column .block-addbysku .block-content:before,
+  .column .block-addbysku .block-content:after,
   .cart-container:before,
   .cart-container:after,
   .login-container:before,
@@ -174,6 +179,7 @@
   .abs-add-clearfix-desktop:after,
   .abs-pager-toolbar:after,
   .block-cart-failed .block-content:after,
+  .column .block-addbysku .block-content:after,
   .cart-container:after,
   .login-container:after,
   .account .column.main .block:not(.widget) .block-content:after,

Commit 4

The definition .abs-rating-summary only existed in the Luma theme, but was being used in the Blank theme, so that didn't work. This definition is now copied over to the Blank theme so it can work again. This results in the stars of accepted product ratings to be vertically aligned.
Also changed the comment above the definition in the Luma theme, as it was incorrectly copy/pasted from the block above.

The diff in the generated css after the fourth commit is:

$ diff -ur before/pub/static/ after/pub/static/
diff -ur before/pub/static/frontend/Magento/blank/en_US/css/styles-m.css after/pub/static/frontend/Magento/blank/en_US/css/styles-m.css
--- before/pub/static/frontend/Magento/blank/en_US/css/styles-m.css   2019-08-11 13:07:07.000000000 +0200
+++ after/pub/static/frontend/Magento/blank/en_US/css/styles-m.css    2019-08-11 13:12:10.000000000 +0200
@@ -2043,6 +2043,20 @@
 .price-excluding-tax .cart-tax-total-expanded:after {
   content: '\e621';
 }
+.review-ratings .rating-summary {
+  display: table-row;
+}
+.review-ratings .rating-label {
+  display: table-cell;
+  padding-bottom: 5px;
+  padding-right: 25px;
+  padding-top: 1px;
+  vertical-align: top;
+}
+.review-ratings .rating-result {
+  display: table-cell;
+  vertical-align: top;
+}
 .block-minicart .subtotal .label:after,
 .minicart-items .details-qty .label:after,
 .minicart-items .price-minicart .label:after,

Commit 5

The definition .abs-account-title only existed in the Luma theme, but was being used in the Blank theme, so that didn't work. This definition is now copied over to the Blank theme so it can work again. This results in the titles on certain pages to be styled slightly differently (bigger font-size, more margin-bottom), modules affected: Magento_GiftRegistry, Magento_MultipleWishlist & Magento_Multishipping
After copying, I've changed the variable in the definition for the border color from @account-title-border-color to @border-color__base as the former doesn't exist in the Blank theme.

The diff in the generated css after the fifth commit is:

$ diff -ur before/pub/static/ after/pub/static/
diff -ur before/pub/static/frontend/Magento/blank/en_US/css/styles-m.css after/pub/static/frontend/Magento/blank/en_US/css/styles-m.css
--- before/pub/static/frontend/Magento/blank/en_US/css/styles-m.css   2019-08-11 13:07:07.000000000 +0200
+++ after/pub/static/frontend/Magento/blank/en_US/css/styles-m.css    2019-08-11 13:20:38.000000000 +0200
@@ -2043,6 +2043,39 @@
 .price-excluding-tax .cart-tax-total-expanded:after {
   content: '\e621';
 }
+.form-giftregistry-search .legend,
+.block-wishlist-search-form .block-title,
+.multicheckout .block-title,
+.multicheckout .block-content .title {
+  border-bottom: 1px solid #d1d1d1;
+  margin-bottom: 25px;
+  padding-bottom: 10px;
+}
+.form-giftregistry-search .legend > strong,
+.form-giftregistry-search .legend > span,
+.block-wishlist-search-form .block-title > strong,
+.block-wishlist-search-form .block-title > span,
+.multicheckout .block-title > strong,
+.multicheckout .block-title > span,
+.multicheckout .block-content .title > strong,
+.multicheckout .block-content .title > span {
+  font-size: 2.2rem;
+  font-weight: 300;
+}
.review-ratings .rating-summary {
  display: table-row;
}

Fixed Issues (if relevant)

Less compilation extend 'mixin' has no matches #23619: Less compilation extend 'mixin' has no matches

Manual testing scenarios (*)

Have nodejs installed (I'm using v8.16.0)
Install the latest version of the less.js compiler in the project (3.9.0 at the time of writing): npm install less@3.9.0
Remove temporarily created frontend files (if any): rm -R var/view_preprocessed/* pub/static/*
Generate intermediate files before less compilation can happen: bin/magento dev:source-theme:deploy --theme=Magento/blank --locale=en_US
Now lint the styles-m.less file using the less.js compiler: node_modules/.bin/lessc -l var/view_preprocessed/pub/static/frontend/Magento/blank/en_US/css/styles-m.less
Expected is to not get any errors, before this fix it would output the errors:

extend ' .abs-modal-overlay' has no matches
extend ' .abs-add-clearfix-desktop' has no matches
extend ' .abs-blocks-2columns' has no matches
extend ' .abs-no-display-desktop' has no matches
extend ' .abs-clearfix' has no matches
extend ' .abs-account-title' has no matches
extend ' .abs-rating-summary' has no matches

Also do a full rm -R var/view_preprocessed/* pub/static/*; bin/magento setup:static-content:deploy -f --theme=Magento/blank --theme=Magento/luma --theme=Magento/backend en_US and compare the changes between the resulting css files without and with these fixes, only changed css files should be in the Blank theme, no other theme's css files should have changed
Then look if these changes didn't cause regressions on the frontend when the Blank theme is being used
- You can check in the frontend if the selector .opc-estimated-wrapper is still being hidden when the screens width >= 768px after this change on the checkout page:
- Somebody with access to Magento Commerce, should look at what happens to the following selectors and see if these are now being rendered properly (before the fix there should be something wrong with the layout I'm assuming):
  - .column .block-addbysku .block-content .box
  - .column .block-addbysku .block-content .box:nth-child(odd)
  - .column .block-addbysku .block-content .box:nth-child(even)
  - .column .block-addbysku .block-content:before
  - .column .block-addbysku .block-content:after
- Have some accepted product review with ratings and see if the stars are vertically aligned:
- Certain titles in the modules Magento_GiftRegistry, Magento_MultipleWishlist & Magento_Multishipping should be styled a bit differently, the font-size should be 2.2rem instead of 1.8rem and the margin-bottom increases from 15px to 25px.
  I could only test this with the Magento_Multishipping module, the other two are part of Magento Commerce of which I don't have an installation running here. So those should be checked as well I assume?
  
  Before:
  
  After:

Questions or comments

It would be nice if Magento could get some automated test which uses the less.js compiler linting mode to detect these problems.
Since one of these compilation problems was recently introduced in the Luma theme code, it probably can't hurt to do some automated testing to prevent this from happening again in the future.
See Manual testing scenario above for the commands needed to detect this.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

m2-assistant · 2019-08-03T09:21:41Z

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

ihor-sviziev · 2019-08-06T07:29:59Z

Hi @hostep,

The selectors .abs-account-title & .abs-rating-summary are being defined in the Magento Luma theme, but were being used in the Magento Blank theme, so this never worked, removed the selectors but not sure if this is the correct solution, maybe the definitions should be moved in the Blank theme as well? (see below under Questions)

I think it should affect styles of luma theme as it extends from blank. If so - looks like definitions should be moved to blank.

hostep · 2019-08-06T20:10:15Z

Hi @ihor-sviziev

Thanks for the feedback!
I think it makes sense indeed to move the definition to the Blank theme.

For the .abs-rating-summary definition it is probably fine to move it over completely.

But I'm having some doubts about the .abs-account-title definition.
For example @account-title-border-color is not defined in Blank, only in Luma. So I'm not exactly sure which lines can be moved over and which one should be restricted to Luma only.
Are there some rules on that maybe? What sort of styling can be applied in Blank and what not? As little colours as possible I assume?

I'll try to do some more digging somewhere in the next few days myself, see how the other less code in Blank and Luma work exactly.

… in the lib code while it was being defined in the Magento/backend theme.

…agento_AdvancedCheckout module. This should in theory improve the frontend layout rendering as to how it was intended.

…me so they can both use it.

…hey can both use it.

hostep · 2019-08-11T11:37:06Z

@ihor-sviziev, I've amended my second commit and restored the removal of the usage of .abs-account-title and .abs-rating-summary. I've added a fourth and fifth commit where those get copied over to the Blank theme. Only moving those over would result in the Luma theme no longer being able to find those, so it needed to be copied and not moved.
Also I've chosen to use the color @border-color__base in the Blank theme instead of the @account-title-border-color from Luma. The second one doesn't exist in the Blank theme and the first one seems to be used a lot in various border colours in Blank. The result is the same though, both those variables result in the color #d1d1d1.

I've then rebased on the latest 2.3-develop branch and amended some commit messages, and force pushed the whole thing back here.

I've also edited the opening post with all this new information.

Phew, a lot of work but I think most of it should be ok now! 🙂

magento-engcom-team · 2019-08-12T09:29:37Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5594 has been created to process this Pull Request

engcom-Alfa · 2019-08-19T10:06:51Z

✔️ QA Passed

Before:

After:

.opc-estimated-wrapper is hidden when the screens width >= 768px

Rating stars are vertically aligned

The font-size for block-title is 2.2rem and the margin-bottom is 25px

#24001

m2-assistant · 2019-08-20T20:40:17Z

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Area: Design/Frontend Area: Lib/Frontend Release Line: 2.3 labels Aug 3, 2019

This was referenced Aug 3, 2019

Fixes less compilation problems in the Magento/luma theme #24003

Merged

[WIP] Fixes a bunch of less compilation problems and resolves some sm… #23942

Closed

ihor-sviziev self-assigned this Aug 6, 2019

ghost added the Progress: needs update label Aug 9, 2019

hostep added 5 commits August 11, 2019 12:42

Fixes a less compilation problem with '.abs-modal-overlay' being used…

cbcb8c6

… in the lib code while it was being defined in the Magento/backend theme.

Fixes some less compilation problems in Magento's blank theme.

ed98703

Fixes a less compilation in Magento's blank theme in regards to the M…

beeb56c

…agento_AdvancedCheckout module. This should in theory improve the frontend layout rendering as to how it was intended.

Copied the definition of '.abs-rating-summary' from Luma to Blank the…

d8c86b3

…me so they can both use it.

Copied the definition of '.abs-account-title' from Luma to Blank so t…

937b2c0

…hey can both use it.

hostep force-pushed the fix-for-issue-23619-blank-theme branch from a26c803 to 937b2c0 Compare August 11, 2019 11:30

ihor-sviziev approved these changes Aug 12, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Aug 12, 2019

ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Aug 12, 2019

engcom-Alfa self-assigned this Aug 19, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Aug 19, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Aug 19, 2019

engcom-Foxtrot self-assigned this Aug 19, 2019

ghost removed the Progress: extended testing label Aug 19, 2019

ghost added the Progress: accept label Aug 19, 2019

magento-engcom-team added the Award: bug fix label Aug 19, 2019

magento-engcom-team mentioned this pull request Aug 20, 2019

Less compilation extend 'mixin' has no matches #23619

Closed

magento-engcom-team pushed a commit that referenced this pull request Aug 20, 2019

ENGCOM-5594: Fixes less compilation problems in the Magento/blank theme

93e2bbd

#24001

magento-engcom-team merged commit 937b2c0 into magento:2.3-develop Aug 20, 2019

magento-engcom-team added this to the Release: 2.3.4 milestone Aug 20, 2019

hostep mentioned this pull request Sep 6, 2021

Less code isn't 100% valid magento/magento2-page-builder#775

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes less compilation problems in the Magento/blank theme #24001

Fixes less compilation problems in the Magento/blank theme #24001

hostep commented Aug 3, 2019 •

edited

Loading

m2-assistant bot commented Aug 3, 2019

ihor-sviziev commented Aug 6, 2019

hostep commented Aug 6, 2019

hostep commented Aug 11, 2019

magento-engcom-team commented Aug 12, 2019

engcom-Alfa commented Aug 19, 2019

m2-assistant bot commented Aug 20, 2019

Fixes less compilation problems in the Magento/blank theme #24001

Fixes less compilation problems in the Magento/blank theme #24001

Conversation

hostep commented Aug 3, 2019 • edited Loading

Description (*)

Commit 1

Commit 2

Commit 3

Commit 4

Commit 5

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Aug 3, 2019

ihor-sviziev commented Aug 6, 2019

hostep commented Aug 6, 2019

hostep commented Aug 11, 2019

magento-engcom-team commented Aug 12, 2019

engcom-Alfa commented Aug 19, 2019

m2-assistant bot commented Aug 20, 2019

hostep commented Aug 3, 2019 •

edited

Loading