Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend does not assume nested selectors #1730

Closed
zanona opened this issue Dec 12, 2013 · 25 comments
Closed

Extend does not assume nested selectors #1730

zanona opened this issue Dec 12, 2013 · 25 comments

Comments

@zanona
Copy link

zanona commented Dec 12, 2013

It seems the feature “Extend” does not assume nested selectors:

This doesn't work

body {
   div { color: red; }
   p:extend(div) { background-color: green; }
}

This does work

body {
   div { color: red; }
   p:extend(body div) { background-color: green; }
}

This also does work

div { color: red; }
p:extend(div) { background-color: green; }

please notice that I had to declare body div on the second example for the feature to work where it should assume its inheritance automatically and the first example should work without problem, but unfortunately that doesn't happen.

@Soviut
Copy link

Soviut commented Dec 12, 2013

You need to use the all keyword.

p:extend(div all) {
    ...
}

@zanona
Copy link
Author

zanona commented Dec 12, 2013

hey @Soviut, I guess it doesn't work on this case because div is inside body. and I just want to inherit styles from div and not body. So even if you use all in the first example you will see it still won't work.

@jonschlinkert
Copy link
Contributor

Actually, you should be extending body div. In other words, the browser doesn't assume nested selectors either, you have defined a nested selector first. So if you extend div that's all you're extending. body div is an entirely different selector.

@zanona
Copy link
Author

zanona commented Dec 13, 2013

@jonschlinkert I am taking into consideration that on the first example p is already nested into body as div is as well. So using the same pattern LESS provides, there would be no need for me to extend body div since p is already nested inside body.

@jonschlinkert
Copy link
Contributor

No, in your first example you're doing p:extend(div) { background-color: green; }, which extends div, but not body div, which is a different, more specific selector. I doesn't matter that p is nested in body.

Expanded out, you have:

body div { 
  color: red; 
}
body {
  p:extend(div) { 
    background-color: green;
  }
}

So you need to do:

body div { 
  color: red; 
}
body {
  p:extend(body div) { 
    background-color: green;
  }
}

@zanona
Copy link
Author

zanona commented Dec 13, 2013

I believe it does matter p is inherited inside body, this is how LESS works for any other selector and it is rather confusing. If you see it like this and how LESS would compile selectors

body {
   div { color: red; } /*selectors are compiled to `body div`*/
   p { border: 1px solid red; } /*selectors are compiled to `body p`*/
   p:extend(div) { background-color: green; } /*while `body p` is transposed ok why `div` is not translated to `body div` *automatically? it should because it is all inside `body`*/
}

Also on my first example the code is almost the same as above and it is all nested within body so it will generate selectors preceded by body i.e: body div, body p, etc?

@jonschlinkert
Copy link
Contributor

I do understand what you mean, you're expecting to be able to "shortcut" over to a selector that is nested inside the same block. and I can see why that might seem appealing. But LESS actually doesn't work that way anywhere else. Selectors are still specified the same way they are in CSS, regardless of being nested together.

This isn't exactly the same, but it might help with the logic. Consider objects and properties in JavaScript. You can't do this:

var obj = {
  foo: 'one',
  bar: foo
};

But you can do obj.bar = obj.foo.

@zanona
Copy link
Author

zanona commented Dec 13, 2013

Does this applies for LESS command line interface as well?
Because the way you have explained it seems it is not possible to perform the selectors on the first example at all on less.js by saying that LESS doesn't work that way anywhere else?

Perhaps I am now getting what you are trying to say. Which is: extend is a method called after LESS has compiled the selectors, just like the javascript example you have showed before?

If that is the case, then I now can understand why it will never find div because it has been compiled to body div.

@jonschlinkert
Copy link
Contributor

I'm just saying you need to specify the selector you want to extend. Nothing more.

@zanona
Copy link
Author

zanona commented Dec 13, 2013

@jonschlinkert thanks man. I have assumed that on my question, but just wasn't sure why this was happening.
Good to know what causes things in such fashion though. Thanks again for the heads up.

@lukeapage
Copy link
Member

I wouldn't want to change the current behaviour as its a breaking change. also forcing the current selector into what you are extending restricts what you are allowed to extend.. where as at the moment it is flexible, though in this usecase you have to repeat the parent selector..

@zanona
Copy link
Author

zanona commented Dec 18, 2013

@lukeapage what happens in the case of the following example tho? I am not sure if that is as flexible as you have mentioned.

div { color: red; }

p {
  @media (max-width:960px) { &:extend(div); }  
}
/*or*/
@media (max-width:960px) {
    p:extend(div) { background: green; }
}

Please note that there is no way to extend anything inside a @media block. No matter what selectors are used, both p extensions above won't work.

@seven-phases-max
Copy link
Member

@zanona

Please note that there is no way to extend anything inside a @media block.

This is how it is supposed to be just by definition of the extend. I.e.:

div {color: red}
// ...
p:extend(div) {}

is just a shortcut for:

div, p {color: red}

How can we inject any @media into this? There's no way. So when it comes to nesting things inside @media blocks a mixin technique is the only way to get (pseudo)-inheritance.

P.S. Probably some kind of warning (again?! :) would be handy when an extend statement fails to do anything, but for now it is as it is.

@seven-phases-max
Copy link
Member

Btw, I'm not on re-opening this issue but I think I have an argument to support the initial example. I guess this issue is a bit wider than it seems to be, it can be re-phrased to "Should extend respect its scope or should it not?" :)
I.e. considering that we have:

body {
   .div {color: red;}
   .div;     // OK
   p {.div;} // OK
}

A one (not yet familiar with how extend actually works) may expect the extend statement to take current scope into account too, e.g. as in the initial example:

body {
   div { color: red; }
   // so if I used just 'div' as mixin why should I use 'body div' with extend?
   p:extend(div) { background-color: green; } 
}

In general both ways make their sense (but I know that implementing "current scope aware extend" would be quite non-trivial so there's no real choice). For us the confusion may be not so evident just because we already know how exactly it works.

Perhaps we could mention this in the docs? E.g. "extend is not current scope aware" or something? (I don't know what would be the best way to say this: "a:extend(b) never considers 'b' scope to be relative to the 'a' scope". I.e. similar to 'relative path' vs. 'absolute path': extend's 'path' is always absolute).

@zanona
Copy link
Author

zanona commented Dec 18, 2013

@seven-phases-max that's sounds a good idea. I am in favour because for me, knowing a little bit of LESS mostly because of its nested selectors feature. This got me by surprise and let me a bit confused.

@lukeapage
Copy link
Member

Lets put it in the docs. As I said, scope aware extend would screw over this more normal case

.a {
  color: black;
}
.b {
  .c:extend(.a) {
    color: green;
  }
}

We could support & in the extend argument, e.g.

.b {
  .a {
    color: black;
  }
  .c:extend(& .a) {
    color: green;
  }
}

but I'm not sure its worth it if people just understand its an absolute selector. Re-open this if you think its worth implementing something like this.

@donaldpipowitch
Copy link

I have to say I would have expected the same behavior as @zanona.

I didn't know this before, but I think it is a major drawback that I can't use :extend inside a media query. I thought that my :extend would be wrapped inside a media query.

E.g. take this

.test-1 {
  background: red;
}

.test-2 {
  background: green;
}

.container-1:extend(.test-1);
.container-2:extend(.test-1);

@media (max-width:960px) {
  .container-1:extend(.test-2);
  .container-2:extend(.test-2);
}

and compile to

.test-1,
.container-1,
.container-2 {
  background: red;
}

.test-2 {
  background: green;
}

@media (max-width:960px) {
  /* .container-1 and .container-2 get the style from .test-2.
     .container-1 and .container-2 are grouped, but still wrapped
     inside a media query and separated from .test-2. */
  .container-1,
  .container-2 {
    background: green;
  }
}

@jonschlinkert
Copy link
Contributor

media queries are a whole different issue

@zanona
Copy link
Author

zanona commented Dec 18, 2013

I guess the proposal behind extend concept is really good, However it is misleading in the way that works adopting a whole different pattern. To be honest, I would rather stay off it and use mixins or just define my selectors manually instead. At least for now.

I guess in a way, extend makes us to think of LESS more of a programming language than a CSS pre-processor. It's easy for users to think it works in such manner because it is really the impression we have while using it.

As @seven-phases-max mentioned: it is easy for the folks behind the project to see it through because they are used with how things work in the backend. So as for @media being a different issue, I'm afraid it still presenting the same problem whilst limiting nested selectors.

I may have got the wrong impression with this and I am sorry if I did, but for me it seems extend may be a feature that will either follow the same pattern as other LESS implementations or it will eventually be cut-off in the future in favour of mixins or something like that.

@lukeapage
Copy link
Member

This issue specifically the case in the description will not be changed..
the behavior is stable.

Media queries are in a different bug and we understand its a shortcoming
and we want to find a solution.

@jonschlinkert
Copy link
Contributor

I guess the proposal behind extend concept is really good, However it is misleading in the way that works adopting a whole different pattern.

I disagree that it's misleading, since it's consistent with how selectors work in general, but I think there is merit to what you're suggesting. So how about if we reopen this as a feature request for having "scoped selectors" work similarly to scoped variables in mixins? @lukeapage?

@seven-phases-max
Copy link
Member

Btw., just in case, using & as "relativity" keyword is potentially ambiguous:

.b {
    .a {
        color: black;
    }

    .e {
        &.c:extend(& .a) {
            color: green;
        }
    }
}

I.e. if & in extend means same thing as & within selector it won't solve the problem.
And if & in extend is just a "relativity mark" (e.g. not a parent selector) it becomes not so intuitive and confusing.

@matthew-dean
Copy link
Member

IIRC, the interpretation of what selectors actually are was one of the contention points when we were developing the :extend feature. Some felt that the way to think about selectors was their Less scoped state, and some thought of selectors as their "compiled" state. I think I typically think about them more like @zanona, that a Less-feature makes sense to apply to the Less selector as written, and not the compiled CSS selector that they may become but does not exist yet.

That is, in @zanona's example:

body {
   div { color: red; }
}

body div { } does not exist yet, as far as the .less file definition. It exists in the mind of the developer, who knows what the code will (may) compile to. So it makes some sense for developers to think of extend as applying only to the explicit written Less selector (and reasonably, to be scoped).

That's not to say anything should change about it, especially after how much debate it went through, just to illustrate that we've always had developers think about their Less code in basically two different ways.

If you like an evening of reading, you can check out Issue #1155. Ultimately, we went with the simplest, most pragmatic approach to :extend that treated .a { .b { } } and .a .b { } as equal. However, we also developed the option of additional keywords which could allow more extending capability. Thus far, I think only "all" was implemented out of "exact, shallow, deep, any, all". (Correct?) Therefore, there's always the possibility that we could add "scoped" to alter the behavior of extend, since we built that flexbility in the spec in the first place.

As far as extend not working inside of @media, I thought that was resolved per #1165 ?

@seven-phases-max
Copy link
Member

As far as extend not working inside of @media, I thought that was resolved per #1165 ?

No, it's considered as expected and documented behaviour (extend does not leave its @media scope). (There's (limited) trick to achieve "global" extending within media block though).

@matthew-dean
Copy link
Member

extend does not leave its @media scope

Right. So, it's working then. @zanona made the comment that it wasn't working, so I took that to mean that he was saying it wasn't supported at all in a @media block and I was confused.

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

No branches or pull requests

7 participants