Skip to content
This repository was archived by the owner on Jan 16, 2025. It is now read-only.

[BUGFIX] Fix @extend an outer selector from within @media #185

Closed

Conversation

marvinhuebner
Copy link

Fixes #87

This fixes two erros with Error: You may not @extend an outer selector from within @media..(#87 (comment)).

@@ -133,7 +133,7 @@ $checkout-tooltip-content-mobile__top : 30px + $checkout-tooltip-icon-
@include max-screen($checkout-tooltip-breakpoint__screen-m) {
.field-tooltip {
.field-tooltip-content {
@extend .abs-checkout-tooltip-content-position-top-mobile;
@include abs-checkout-tooltip-content-position-top();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to directly include the mixin here would be to use $screen__m + 1 instead of $checkout-tooltip-breakpoint__screen-m as it is used here: https://github.com/SnowdogApps/magento2-theme-blank-sass/blob/master/styles/blocks/_extends.scss#L1132-L1136

Then the @extend could persist. But then the media query value would not longer inherit from $modal-popup-breakpoint-screen__m.

@Igloczek
Copy link
Contributor

Igloczek commented Mar 13, 2018

Remember to follow the contribution guidelines and fix this bug also in the magento2 repository or provide a proof that was just a mistake translating LESS to SASS.
We don't want to create an alternative version of the Blank theme, it needs to be to same no matter if you are usong LESS or SASS

@marvinhuebner
Copy link
Author

In the Magento blank theme this is working because LESS is less strict as SASS.

I fully agree that this blank theme needs to do the same as the Magento LESS Theme does. But how to handle the internal differences between LESS and Sass?

To verify that the error stays not in connection with frontools i also tested to compile the theme with node-sass@4.8.2 (node-sass styles/styles.scss -o styles) and the command line compiler. But if i do it this way, i also get the same erros:

{
  "status": 1,
  "file": "/Users/marvinhuebner/Projekte/wot/multistore/vendor/snowdog/theme-blank-sass/styles/blocks/_extends.scss",
  "line": 1133,
  "column": 5,
  "message": "You may not @extend an outer selector from within @media.\nYou may only @extend selectors within the same directive.\nFrom \"@extend .abs-checkout-tooltip-content-position-top-mobile\" on line 137 of Magento_Checkout/styles/module/checkout/_tooltip.scss\n",
  "formatted": "Error: You may not @extend an outer selector from within @media.\n       You may only @extend selectors within the same directive.\n       From \"@extend .abs-checkout-tooltip-content-position-top-mobile\" on line 137 of Magento_Checkout/styles/module/checkout/_tooltip.scss\n        on line 1133 of styles/blocks/_extends.scss\n>>     .abs-checkout-tooltip-content-position-top-mobile {\n\n   ----^\n"
}

So if i understand you correct we have to patch the Magento LESS blank theme so, that it can work the same way in the SASS theme?

I would love to get this fixed in some way, because if i install the blank theme in the current version i can not compile the SASS before i patch this two files.

@Igloczek
Copy link
Contributor

I want to prevent a situation where translating LESS code from M2 repository will bring bugs to the SASS implementations, just b/c LESS is okay with stupid solutions.

So you need to submit PR to the M repository to reflect changes done here.

https://github.com/magento/magento2/blob/2.2-develop/app/design/frontend/Magento/blank/Magento_Checkout/web/css/source/module/checkout/_tooltip.less#L144
https://github.com/magento/magento2/blob/2.2-develop/app/design/frontend/Magento/blank/Magento_Sales/web/css/source/_module.less#L265

dnsv and others added 2 commits March 13, 2018 16:40
…owdogApps#180)

* Add default flag to all variables defined inside Magento_Checkout

* Add default flag to all variables
@marvinhuebner
Copy link
Author

marvinhuebner commented Mar 14, 2018

I will close this for now, because i've installed a new vendor and now it works out of the box. I can not explain it to me. Will keep an eye on it.

But thanks for your input and the insights to the SASS port.

@Igloczek
Copy link
Contributor

@talalus let's take this over, I believe we need this fix

@rglepper
Copy link

Hi guys, I just wanted to point out that this error is expected since in Sass it's not allowed to use @extend within directives unless the extended selector is within the same block http://sass-lang.com/documentation/file.SASS_REFERENCE.html#extend_in_directives I don't quite understand why this was compiling without errors, but this is definitely a bug.

@marvinhuebner
Copy link
Author

I don't quite understand why this was compiling without errors, but this is definitely a bug.

@rglepper i've also no clue why this works. Yesterday we configured our CI/CD for our new project and the build job failed everytime when compiling the SASS with the same error mentioned above. Then we switched from npm to yarn install to install the dependencies depending on the yarn.lock file and then it works.

@rglepper
Copy link

@marvinhuebner I think this could be related to the version of libsass being used by node-sass http://sass-compatibility.github.io/#cross_media_extend I know that it doesn't work with the current version of node-sass 4.8.3 and this is the correct behaviour

@marvinhuebner
Copy link
Author

@rglepper can confirm. If i update to node-sass 4.8 it breaks. In node-sass 4.8, libsass is upgraded to the latest 3.5 release.

So how can we deal with this? I can not image how i can "fix" this in the less version of the base theme, because if i argue with: "Sass is stricter then less and in sass this does not work anymore, we have to change this in the base theme so the sass theme can adopt this change and the sass compiler will work again".

Personally I think that a patch for the less base theme is hard to become in.

@Igloczek
Copy link
Contributor

Fixed in 1.0.5

Changes are reflected in magento/magento2#14395

@marvinhuebner
Copy link
Author

@Igloczek Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with @extend inside @media
4 participants