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

Add Sass like extend #509

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@hokaccha
Contributor

hokaccha commented Dec 5, 2011

Sass extend is here.
http://sass-lang.com/docs/yardoc/file.SASS_REFERENCE.html#extend

More simple syntax, use +.

.foo {
  width: 100px;
}
.bar {
  +.foo;
}

converted this.

.foo, bar {
  width: 100px;
}

See also test code.

@cloudhead

This comment has been minimized.

Member

cloudhead commented Dec 7, 2011

Nice work, I'll have a look!

@madevelopers

This comment has been minimized.

madevelopers commented Dec 12, 2011

and the output is also grouped. nice.

@cloudhead

This comment has been minimized.

cloudhead commented on lib/less/tree/extend.js in 3722e83 Dec 13, 2011

The ; here should be a ,.

This comment has been minimized.

Owner

hokaccha replied Dec 13, 2011

@aziz

This comment has been minimized.

aziz commented Mar 31, 2012

I really like to see this one get merged into LESS. It's obviously missing this feature and will help us generate lighter and leaner css files by reusing instead of repeating.

a big 👍

@jmgunn87

This comment has been minimized.

jmgunn87 commented Apr 29, 2012

Yeah, I like this as well. Is this going to be merged or what?

@davidhund

This comment has been minimized.

davidhund commented May 16, 2012

I also would love to know if/when this is going to be implemented.

@rowanmanning

This comment has been minimized.

rowanmanning commented May 30, 2012

It'd be awesome to get something like this into LESS. Any plans to merge?

@icholy

This comment has been minimized.

icholy commented Jun 15, 2012

+1

@kadamwhite

This comment has been minimized.

kadamwhite commented Jun 15, 2012

Another +1 to this, it'd be a huge help!

@albohlabs

This comment has been minimized.

albohlabs commented Jun 15, 2012

Nice one. +1

@sturobson

This comment has been minimized.

sturobson commented Jun 26, 2012

+1 can we get this merged into LESS please. It'll help loads, really.

@chrisui

This comment has been minimized.

chrisui commented Jun 26, 2012

+1 I feel this is the one major piece of functionality LESS is missing.

(Also: more +1's from another issue: #759)

@icholy icholy referenced this pull request Jun 26, 2012

Closed

Selector Inheritance #759

@json-uk

This comment has been minimized.

json-uk commented Jul 9, 2012

Has there been any movement on this? Why hasn't the developer pulled this in? All the comparisons between SASS and LESS point to this as one of the killer features. Cloudhead your community desparatwly wants this feature!!!!

@chrisui

This comment has been minimized.

chrisui commented Jul 9, 2012

Any chance of taking a look @cloudhead ?

@json-uk

This comment has been minimized.

json-uk commented Jul 20, 2012

Can someone with prove ledges please add this and release it? This pull request is months old. The LESS community will be very grateful!

@lukeapage

This comment has been minimized.

Member

lukeapage commented Jul 20, 2012

we will get round to considering this, but I think bugs are higher priority.

@kadamwhite

This comment has been minimized.

kadamwhite commented Jul 21, 2012

I can't disagree with the importance of bug fixes, @agatronic, but I'd also second @dtigraphics that time and time again I hear people recommend Sass over LESS because LESS lacks this feature... It's a useful bit of functionality that would really improve the experience of using the tool, IMO, and we'd love to see this prioritized as soon as new features are under discussion.

In the meantime, what can we as a community do to test/support this request? There's a lot of us on this thread by now, if there is anything we can do to speed up the inclusion of this feature we would love to help!

@leopic

This comment has been minimized.

leopic commented Jul 21, 2012

If you want this REALLY bad you could just fork the repo and make your own build in the mean time ;)

@icholy

This comment has been minimized.

icholy commented Jul 21, 2012

@leopic and then have to go back and fix all your code when a slightly modified version of this feature gets merged.

@leopic

This comment has been minimized.

leopic commented Jul 21, 2012

Well you can't have your cake and eat it too, IMHO the whole idea of exposing your source code is that people don't have to wait 8 months before trying out a new feature gets merged, as long as you can make a build you can run your own flavor of your favorite tool =)

@icholy

This comment has been minimized.

icholy commented Jul 21, 2012

@leopic that's true when you're hacking at personal projects. But maintaining a patch like this in production quickly turns into a huge pita.

@json-uk

This comment has been minimized.

json-uk commented Jul 21, 2012

I think it's clear that there's a very large community of developers who use, and would like to help the project to keep it alive. Let's be honest - the ratio of open bugs to closed is staggering. It's starting to appear as though the project has lost momentum. What can this community do in order to keep this easy to use and powerful tool alive?

You brought up a good point about the difference between personal and public projects. It's important to maintain momentum to ensure LESS doesn't fade away like so many other great tools!

Jason Chandler
jc@designtoinspire.net
http://dtigraphics.net
Design, the way nature intended.

On Jul 20, 2012, at 10:46 PM, ilia choly reply@reply.github.com wrote:

@leopic that's true when you're hacking at personal projects. But maintaining a patch like this in production quickly turns into a huge pita.


Reply to this email directly or view it on GitHub:
#509 (comment)

@jmgunn87

This comment has been minimized.

jmgunn87 commented Jul 21, 2012

Well let's fork and get on with it. Who's first?

@lukeapage

This comment has been minimized.

Member

lukeapage commented Jul 22, 2012

before you do that please read bug #867 and move this conversation there. we are doing our best!

@superfunkminister

This comment has been minimized.

superfunkminister commented Aug 21, 2012

so can this happen now? would be SO AWESOME

@kadamwhite

This comment has been minimized.

kadamwhite commented Aug 30, 2012

@agatronic - Thanks for all the work you and the others are doing to maintain this project. Please let us know if we can do anything to vet out this PR... While it is a new feature/increased scope, I have semi-regularly heard people express the lack of extend as the sole reason they are choosing SCSS over LESS and I'd love to be involved in the conversation about how we might address that issue.

@Soviut

This comment has been minimized.

Soviut commented Nov 1, 2012

The documentation can list "extending" and "extension" to make the
association more concrete when searching.

On Oct 31, 2012, at 6:50 PM, "Mária Jurčovičová" notifications@github.com
wrote:

I may be wrong, bug googling "less extend" will probably find less
documentation while googling "less ++" will probably find a lot of random
sites. Plus, if it is named extend I can safely guess what it does without
googling it or remembering special symbols table.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/509#issuecomment-9965104.

@Soviut

This comment has been minimized.

Soviut commented Nov 1, 2012

+1 for :extend

On Oct 31, 2012, at 7:16 PM, matthewdl notifications@github.com wrote:

@jonschlinkert https://github.com/jonschlinkert Your proposed syntax is
pretty close to what I was envisioning in my head. It feels more elegant,
and like mixin guards for LESS which mimic media query syntax, it feels
more like an extension of CSS, and therefore, more LESS-like than other
symbols.

Plus, by making :extend in the declaration, it more accurately describes
that this class definition will be merged with other selectors (and which
ones). Having the extend declaration arbitrarily placed somewhere within
the list of properties doesn't feel right for an extending behavior. This
is on par with, say, the ":not" psuedo-class, and some of the jQuery
pseudo-class extensions which mimic CSS.

Very nice work. This syntax, in itself, makes me like the whole concept of
extend even more, because it's much more clearly defined.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/509#issuecomment-9965807.

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 1, 2012

@Soviut - when you reply in emails, click off the quote - then we get cleaner comments :)

@jonschlinkert nice. I like the idea we can use it for both stand alone and as part of the selector. some questions

  1. same as before .a, .b:extend(.c) ... does the extend cover both selectors .a and .b or should it be disallowed?
  2. is there any difference between .b :extend(.c) and .b:extend(.c) ?

If I get some time I'll look at implementing this and also figuring out how to fix #1104.. does every agree with #1104 btw? I haven't had time to figure it out.

I would love it If anyone else would prefer to do the coding, as I have plenty of other less coding to do.

@dpashkevich

This comment has been minimized.

dpashkevich commented Nov 1, 2012

I like @jonschlinkert suggestion too. I wouldn't want the inheritance definition get lost among property definitions. It can still be surrounded by other pseudo-class definitions, though, but then I would probably put them on separate lines like so:

.alert
    :extend(.alert-important, .alert-danger) 
    :first-child
    :hover
    {
    background: red;
    ...
}

Of course the example is rather esoteric and in LESS I should use the & combinator nested inside the main selector instead but this syntax should still be valid, right?

One concern that I have is about pseudo-elements. The following line:

.alert::before:extend(.shiny-tip) {}

Is supposed to mean that the ::before pseudo-element extends .shiny-tip but the syntax is a bit confusing in this case.

P.S. With the Shadow DOM spec we will see way more pseudo-elements coming!

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 1, 2012

Isn't it confusing allowing things after :extend? I have an idea what it
might mean, but it would mean you can extend part of a single selector? I'd
rather disallow that for now.

@Soviut

This comment has been minimized.

Soviut commented Nov 1, 2012

True, the :extend syntax, while CSS in spirit, deviates significantly from the SASS style @extend in terms of implementation.

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 1, 2012

Well, not necessarily but we need to think about these cases. We could
always initially just support the stand alone &:extend( first.

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 2, 2012

@agatronic the point you bring up is valid for straight CSS as well, meaning that it seems confusing. It wouldn't surprise me if a lot of front-end folks didn't know that the spec currently allows for chaining pseudo-selectors at all.

My view (conjecture?) is that if you use pseudo-selectors regularly in your CSS, then you should have very little learning curve here. As for whether or not chaining should be allowed, it seems that it would be difficult to implement without it, at least eventually. Just consider hover and active as prime examples. I think if we view the problem from 20,000 feet, we shouldn't allow the syntax to determine the objective.

It makes the most sense to consider the following as a typical use-case, and a reason to allow chaining:

E:hover:extend(...) {}

We make regular use of these pseudo-selectors:

E:hover {}
E:active {}
E:before {}
E:after {}

So my suggestion is to think if it in that context, rather than thinking of more complicated and less-often used use cases like:

p:first-child:first-letter:first-line:after:hover {}

Which, btw, is valid. But would anyone use it? I guess, yeah, sure if I had to... but it's not a probably not a good use case lol.

That being said, I'm aware that you have to weigh this against other considerations, so it's ultimately up to you whether or not chaining will be introduced in the first iteration of the :extend feature.

My vote is that :extend is still powerful without chaining, but even more useful with it. So do whatever is pragmatic for now..

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 2, 2012

It wasn't chaining that I was concerned with.. it was having the extend in
the middle of the selector.. does it still apply to the whole thing?

@matthew-dean

This comment has been minimized.

Member

matthew-dean commented Nov 5, 2012

To jump in, no, I don't think :extend should be allowed in the middle of the selector. Like jQuery's extensions, it still belongs at the end. It should apply to the entire selector as a string match.

To extend a block like above:

E:hover:extend(...), E:active:extend(...) {
  property: value;
}

// equivalent to:

E:hover, E:active {
  &:extend(...) {
    property: value;
  }
}

This allows you to separate and use LESS in coherent blocks, extending only the properties you want to base classes, while having some properties only be local one-off values, or controlling output order.

E:hover, E:active {
  &:extend(...) {
    property: value;
  }
  non-merged-property: value;
}

In other words, this syntax I think would be simpler to understand and also a bit more powerful than SASS.

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 5, 2012

Sounds good.

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 11, 2012

I've had some time to test :extend in 1_4_0 (using the default test files as well as Bootstrap's Less), and so far it works as expected. Lot's more testing to do, but I'm also looking forward to seeing feedback from others. Thanks for the hard work on this!

@Synchro

This comment has been minimized.

Member

Synchro commented Nov 11, 2012

It would be much appreciated if you could contribute some examples and edge cases to add to the tests.

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 13, 2012

@Synchro no problem, I'd be happy to. I'll try to put something together this week.

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 13, 2012

Check the existing tests.. there are a number already.

Would appreciate any comments on whether less should copy the selector
matching of sass or keep things simple. See the bug I referenced above.

I've still yet to expand it to allow using extend in the selector..

One question on the stand alone...

.b {
  &:extend(.a);
}

Or

.b {
  :extend(.a);
}

Or both?

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 13, 2012

I think only the first example should be valid. &:extend conforms to a known pattern in Less

@Soviut

This comment has been minimized.

Soviut commented Nov 13, 2012

I agree. It should follow the correct selector logic if it's going to be treated as a pseudo-selector. The first example reads as "extend .b with .a" while the second example reads as "extend the decendents of .b".

@matthew-dean

This comment has been minimized.

Member

matthew-dean commented Nov 13, 2012

@Soviut

This comment has been minimized.

Soviut commented Nov 13, 2012

Documentation-wise, I think the .b:extends(.a) syntax should be pushed more prominently though.

@matthew-dean

This comment has been minimized.

Member

matthew-dean commented Nov 13, 2012

@Soviut +1 to that, too.

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 15, 2012

@agatronic and forgot to mention, no problem on the tests either way. I did run the tests included in 1_4_0, everything checks out. If you want me to throw more together as 1_4_0 progresses just say the word

@lukeapage

This comment has been minimized.

Member

lukeapage commented Nov 16, 2012

this is the bug I would appreciate comments on

#1014

@jonschlinkert

This comment has been minimized.

Contributor

jonschlinkert commented Nov 16, 2012

@agatronic will do, I'm putting something together since there is some thought involved here. I'll update as soon as I have something to share.

@feichang

This comment has been minimized.

feichang commented Dec 25, 2012

+1 for extend

@czarnota

This comment has been minimized.

czarnota commented Jan 5, 2013

why not to just steal @extend syntax from SASS?

@lukeapage

This comment has been minimized.

Member

lukeapage commented Jan 5, 2013

Because sass has a different design philosophy. We have already decided the
syntax.

@DesignByOnyx

This comment has been minimized.

DesignByOnyx commented Jan 25, 2013

I have finally gotten around to testing this and wanted to post my initial findings. I have been using the alpha version less-1.4.0-alpha.js on the client-side only (no command line yet - not that it should be any different really).

So far, everything works as I would expect except for two major drawbacks which can be explained with one example. Take the following code which is located in one of my root LESS files:

.container { margin: 0 auto; }
.container.positioned { position: absolute }

@media screen and (min-width: 30rem) {
    .container.positioned { left: 50%; }
}
@media screen and (min-width: 30rem) and (max-width: 48rem) {
    .container { width: 30rem; }
    .container.positioned { margin-left: -15rem; }
}
@media screen and (min-width: 48rem) and (max-width: 60rem) {
    .container { width: 48rem; }
    .container.positioned { margin-left: -24rem; }
}
@media screen and (min-width: 60rem) {
    .container { width: 60rem; }
    .container.positioned { margin-left: -30rem; }
}

Issue 1 - styles defined within media queries do not get extended. Anything trying to extend the .container class only gets the margin: 0 auto styles.

.some-element:extend(.container);

Issue 2 - compound selectors do not get extended. However, the first participant DOES get extended. For example, the following incorrectly extends the .container styles but not the intended .container.positioned styles.

.some-element:extend(.container.positioned);

I wish I could provide a solution. Hope this helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment