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

hostep · 2019-08-03T10:39:25Z

Related Pull Requests

https://github.com/magento/partners-magento2b2b/pull/52

Description (*)

Hi

This PR is fixing less compilation problems in Magento's Luma theme as being reported in #23619
A PR for fixing similar problems in the Blank 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 is mostly relevant in the PR for the Luma theme)

This PR consists of 3 commits.

Commit 1

The first commit is not directly related to the Luma theme, but caused a compilation problem with the Luma 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-action-reset is being defined in the backend theme, but was being used on the frontend, so this never worked, removed its usage on the frontend in this case, was recently introduced by MAGETWO-64838
The selector .clearer is nowhere defined. It stems from the early days of Magento. It got first changed to '.mixin-clearer', and then later to '.lib-clearer', removed the usage of .clearer here since it doesn't seem to be necessary anymore
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/pub/static/frontend/Magento/luma/en_US/css/styles-l.css after/static/frontend/Magento/luma/en_US/css/styles-l.css
--- before/pub/static/frontend/Magento/luma/en_US/css/styles-l.css  2019-08-03 10:45:14.000000000 +0200
+++ after/static/frontend/Magento/luma/en_US/css/styles-l.css 2019-08-03 11:56:03.000000000 +0200
@@ -786,6 +786,7 @@
     text-align: center;
   }
   .abs-no-display-desktop,
+  .opc-estimated-wrapper,
   .sidebar .block.widget .pager .item:not(.pages-item-next):not(.pages-item-previous) {
     display: none;
   }
@@ -2060,9 +2061,6 @@
     float: right;
     margin: 23px 0 0;
   }
-  .opc-estimated-wrapper {
-    display: none;
-  }
   .opc-progress-bar {
     margin: 0 0 20px;
     counter-reset: i;

Commit 3

The third commit should in theory improve the rendering of a certain modules both in Magento Open Source as in Magento Commerce.
Unfortunately I don't have a copy over here at the moment of Magento Commerce to test it all 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 other fixes are (see screenshots below in the Manual testing scenarios):

in the Magento_CatalogSearch module, it now hides the rendering of the legend in the advanced search form (the code wanted to do that, but due to the usage of the .abs-forms-general-desktop being used in the wrong media query, it didn't work out)
in the Magento_Sales module, it now shows a print icon next to print links (the code wanted to do that, but due to the usage of the .abs-forms-general-desktop being used in the wrong media query, it didn't work out)

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/luma/en_US/css/styles-l.css after/pub/static/frontend/Magento/luma/en_US/css/styles-l.css
--- before/pub/static/frontend/Magento/luma/en_US/css/styles-l.css  2019-08-03 10:45:14.000000000 +0200
+++ after/pub/static/frontend/Magento/luma/en_US/css/styles-l.css 2019-08-03 12:05:30.000000000 +0200
@@ -483,6 +483,7 @@
   }
   .abs-blocks-2columns,
   .abs-discount-block-desktop .block,
+  .column .block-addbysku .block-content .box,
   .login-container .block,
   .account .column.main .block:not(.widget) .block-content .box,
   .form-address-edit > .fieldset,
@@ -493,6 +494,7 @@
     width: 48%;
   }
   .abs-discount-block-desktop .block:nth-child(1),
+  .column .block-addbysku .block-content .box:nth-child(1),
   .login-container .block:nth-child(1),
   .account .column.main .block:not(.widget) .block-content .box:nth-child(1),
   .form-address-edit > .fieldset:nth-child(1),
@@ -504,6 +506,7 @@
     float: left;
   }
   .abs-discount-block-desktop .block:nth-child(2),
+  .column .block-addbysku .block-content .box:nth-child(2),
   .login-container .block:nth-child(2),
   .account .column.main .block:not(.widget) .block-content .box:nth-child(2),
   .form-address-edit > .fieldset:nth-child(2),
@@ -514,6 +517,7 @@
     float: right;
   }
   .abs-discount-block-desktop .block:nth-child(2) + *,
+  .column .block-addbysku .block-content .box:nth-child(2) + *,
   .login-container .block:nth-child(2) + *,
   .account .column.main .block:not(.widget) .block-content .box:nth-child(2) + *,
   .form-address-edit > .fieldset:nth-child(2) + *,
@@ -574,6 +578,8 @@
   .order-review-form: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,
@@ -614,6 +620,7 @@
   .paypal-review-discount:after,
   .order-review-form:after,
   .block-cart-failed .block-content:after,
+  .column .block-addbysku .block-content:after,
   .cart-container:after,
   .login-container:after,
   .account .page-title-wrapper:after,
@@ -720,11 +727,13 @@
     width: 100%;
   }
   .abs-forms-general-desktop,
+  .form.search.advanced,
   .form-giftcard-redeem,
   .form-giftregistry-create {
     max-width: 500px;
   }
   .abs-forms-general-desktop .legend,
+  .form.search.advanced .legend,
   .form-giftcard-redeem .legend,
   .form-giftregistry-create .legend {
     border: 0;
@@ -737,6 +746,7 @@
     width: 1px;
   }
   .abs-forms-general-desktop .legend + br,
+  .form.search.advanced .legend + br,
   .form-giftcard-redeem .legend + br,
   .form-giftregistry-create .legend + br {
     display: none;
@@ -765,11 +775,13 @@
   .table-wrapper.orders-recent {
     margin-top: -25px;
   }
-  .abs-action-print {
+  .abs-action-print,
+  .order-actions-toolbar .action.print {
     display: inline-block;
     text-decoration: none;
   }
-  .abs-action-print:before {
+  .abs-action-print:before,
+  .order-actions-toolbar .action.print:before {
     -webkit-font-smoothing: antialiased;
     -moz-osx-font-smoothing: grayscale;
     font-size: 16px;
@@ -785,7 +797,11 @@
     speak: none;
     text-align: center;
   }
+  .order-actions-toolbar .action.print:hover {
+    text-decoration: underline;
+  }
   .abs-no-display-desktop,
   .opc-estimated-wrapper,
   .sidebar .block.widget .pager .item:not(.pages-item-next):not(.pages-item-previous) {

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/luma --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/luma/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-forms-general-desktop' has no matches
extend ' .abs-action-reset' has no matches
extend ' .abs-no-display-desktop' has no matches
extend ' .clearer' has no matches
extend ' .abs-clearfix' has no matches
extend ' .abs-action-print' 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 Luma theme, no other theme's css files should have changed
Then look if these changes didn't cause regressions on the frontend when the Luma 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
- On the advanced search form, you should now no longer see the legend 'Search Settings' being rendered when the width of your screen is >= 768px
- On the order history of a logged in user, you should now see a print icon being shown next to print links when the width of your screen is >= 768px

Questions or comments

I'm wondering if the 'Search Settings' legend in de advanced search form should also be hidden on screens with width < 768px
I feel like it is only there for accessibility reasons and should always be visibly hidden, can that be?

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)

… in the lib code while it was being defined in the Magento/backend theme. (cherry picked from commit 9340950)

…xing: - Magento_CatalogSearch module: hides legend in advanced search form - Magento_Sales module: adds print icon to print link for sales objects in order history - Magento_AdvancedCheckout module: not sure, can't test at the moment

m2-assistant · 2019-08-03T10:39:27Z

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

magento-engcom-team · 2019-08-06T07:24:45Z

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5546 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

ihor-sviziev · 2019-08-06T07:25:51Z

For now added "not covered" label, I think it could be automatically checked with original lessc compiler

engcom-Alfa · 2019-08-06T09:41:27Z

✔️ QA Passed

Before:

After:

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

On the advanced search form "Search Settings" legend is no longer visible when the width of screen is >= 768px

On the order history now we can see a print icon being shown next to print links when the width is >= 768px

sidolov · 2019-08-07T20:57:06Z

Failed B2B functional test

Magento\QuickOrder\Test\TestCase\QuickAddToCartTest::test with data set "QuickAddToCartVariation1_0" (QuickAddToCartVariation1_0)
PHPUnit_Extensions_Selenium2TestCase_WebDriverException: Timed out after 180 seconds

@engcom-Foxtrot please, take a look

engcom-Foxtrot · 2019-08-15T10:33:59Z

@magento run all tests

ihor-sviziev · 2019-09-06T14:09:21Z

Hi @engcom-Foxtrot @sidolov,
Is there any updates?

slavvka · 2020-01-15T21:52:21Z

dev/tests/static/testsuite/Magento/Test/GraphQl/_files/changed_files_magento2.txt is a file generated by Static Tests so it may contain a forbidden word. It is a known test issue that it checks files generated by itself (https://jira.corp.magento.com/browse/MTS-852). No need for an approval

slavvka · 2020-02-09T07:52:44Z

Magento\QuickOrder\Test\TestCase\QuickAddToCartTest.test with data set "QuickAddToCartVariation1_0" is falling constantly on this PR. @hostep Please fix it.

hostep · 2020-02-09T11:00:31Z

@slavvka: that failure is in the functional tests B2B, right? I don't have access to Magento B2B I think so I don't know how to test/fix this.

slavvka · 2020-02-09T23:08:51Z

@hostep oh, you are right.
@engcom-Foxtrot could you please take a look?

engcom-Golf · 2020-02-27T11:09:53Z

I'll take care of failing tests

engcom-Golf · 2020-02-27T13:18:45Z

@magento run all tests
with b2b branch

engcom-Golf · 2020-02-27T20:22:38Z

@slavvka done ✔️
related PR with test fixes https://github.com/magento/partners-magento2b2b/pull/52

magento-engcom-team · 2020-02-27T20:58:33Z

Hi @slavvka, thank you for the review.
ENGCOM-5546 has been created to process this Pull Request

…#24003

m2-assistant · 2020-02-28T22:05:19Z

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.

hostep added 3 commits August 3, 2019 11:36

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

820fd79

… in the lib code while it was being defined in the Magento/backend theme. (cherry picked from commit 9340950)

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

164a053

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/blank theme #24001

Merged

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

Closed

ihor-sviziev approved these changes Aug 6, 2019

View reviewed changes

ghost assigned ihor-sviziev Aug 6, 2019

ghost added the Progress: ready for testing label Aug 6, 2019

ihor-sviziev added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Aug 6, 2019

engcom-Alfa self-assigned this Aug 6, 2019

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

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

engcom-Foxtrot self-assigned this Aug 6, 2019

ghost added Progress: accept and removed Progress: extended testing labels Aug 6, 2019

ghost added Progress: extended testing and removed Progress: accept labels Aug 7, 2019

ghost removed the Progress: extended testing label Nov 28, 2019

ghost added Progress: accept and removed Progress: pending approval labels Jan 15, 2020

magento-engcom-team assigned VladimirZaets Jan 15, 2020

ghost added Progress: needs update and removed Progress: accept labels Feb 9, 2020

engcom-Foxtrot removed their assignment Feb 27, 2020

engcom-Golf self-assigned this Feb 27, 2020

ghost added Progress: review and removed Progress: needs update labels Feb 27, 2020

slavvka approved these changes Feb 27, 2020

View reviewed changes

ghost added Progress: ready for testing Progress: testing in progress Progress: accept and removed Progress: review Progress: ready for testing Progress: testing in progress labels Feb 27, 2020

magento-engcom-team mentioned this pull request Feb 28, 2020

Less compilation extend 'mixin' has no matches #23619

Closed

magento-engcom-team pushed a commit that referenced this pull request Feb 28, 2020

ENGCOM-5546: Fixes less compilation problems in the Magento/luma theme …

6b9183e

…#24003

magento-engcom-team merged commit 02c6691 into magento:2.4-develop Feb 28, 2020

hostep mentioned this pull request Sep 6, 2021

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

Closed

sdzhepa mentioned this pull request May 9, 2022

tyutyu sdzhepa/magento2#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hostep commented Aug 3, 2019 •

edited by engcom-Golf

Loading

m2-assistant bot commented Aug 3, 2019

magento-engcom-team commented Aug 6, 2019

ihor-sviziev commented Aug 6, 2019

engcom-Alfa commented Aug 6, 2019 •

edited

Loading

sidolov commented Aug 7, 2019

engcom-Foxtrot commented Aug 15, 2019

ihor-sviziev commented Sep 6, 2019

slavvka commented Jan 15, 2020 •

edited

Loading

slavvka commented Feb 9, 2020

hostep commented Feb 9, 2020

slavvka commented Feb 9, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

magento-engcom-team commented Feb 27, 2020

m2-assistant bot commented Feb 28, 2020

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

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

Conversation

hostep commented Aug 3, 2019 • edited by engcom-Golf Loading

Related Pull Requests

Description (*)

Commit 1

Commit 2

Commit 3

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Aug 3, 2019

magento-engcom-team commented Aug 6, 2019

ihor-sviziev commented Aug 6, 2019

engcom-Alfa commented Aug 6, 2019 • edited Loading

sidolov commented Aug 7, 2019

engcom-Foxtrot commented Aug 15, 2019

ihor-sviziev commented Sep 6, 2019

slavvka commented Jan 15, 2020 • edited Loading

slavvka commented Feb 9, 2020

hostep commented Feb 9, 2020

slavvka commented Feb 9, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

magento-engcom-team commented Feb 27, 2020

m2-assistant bot commented Feb 28, 2020

hostep commented Aug 3, 2019 •

edited by engcom-Golf

Loading

engcom-Alfa commented Aug 6, 2019 •

edited

Loading

slavvka commented Jan 15, 2020 •

edited

Loading