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

Already on GitHub? Sign in to your account

can't extend a placeholder selector unless it has a root #52

Closed
jonathanong opened this Issue Mar 12, 2013 · 9 comments

Comments

Projects
None yet
2 participants
Contributor

jonathanong commented Mar 12, 2013

%clearfix:before,
%clearfix:after {
  content: " ";
  display: table;
}

%clearfix:after {
  clear: both;
}

.clearfix {
  extend: %clearfix
}

Error:

    Error: failed to extend "%clearfix"
        at visit (/Users/jonathanong/Workspace/discore-css/node_modules/rework/lib/plugins/extend.js:43:24)
        at module.exports (/Users/jonathanong/Workspace/discore-css/node_modules/rework/lib/plugins/extend.js:22:7)
        at Array.forEach (native)
        at module.exports (/Users/jonathanong/Workspace/discore-css/node_modules/rework/lib/plugins/extend.js:16:17)
        at Rework.use (/Users/jonathanong/Workspace/discore-css/node_modules/rework/lib/rework.js:58:3)
        at Object.<anonymous> (/Users/jonathanong/Workspace/discore-css/index.js:7:2)
        at Module._compile (module.js:449:26)
        at Object.Module._extensions..js (module.js:467:10)
        at Module.load (module.js:356:32)
        at Function.Module._load (module.js:312:12)

Then I add a dummy placeholder:

%clearfix {

}

%clearfix:before,
%clearfix:after {
  content: " ";
  display: table;
}

%clearfix:after {
  clear: both;
}

.clearfix {
  extend: %clearfix
}

It compiles, but I get the following:

.clearfix {

}

%clearfix:after,
.clearfix:before {
  content: " ";
  display: table
}

.clearfix:after {
  clear: both
}

.clearfix {

}

Which is not correct.

Contributor

jonathanong commented Mar 14, 2013

i'm trying to figure out how placeholder works, but i'm pretty sure you can't splice inside a forEach without messing it up https://github.com/visionmedia/rework/blob/master/lib/plugins/extend.js#L24

ex.

var a = [0, 1, 2, 3]

// remove 0 and 2

a.forEach(function (x, i) {
  if (i === 0 || i === 2) a.splice(i, 1);
})

// expect [1, 3]
a === [1, 2]

@tj tj reopened this Mar 14, 2013

Owner

tj commented Mar 14, 2013

I agree this should be fixed

Contributor

jonathanong commented Mar 14, 2013

the link i posted above works, but it works differently than extends does. not sure if you want to implement it. specifically:

%placeholder + %placeholder
  color: black

.black 
  extends: %placeholder

yields:

 .black + .black
   color: black

Is this correct to you? I'm not sure what it would yield right now since I haven't tested it.

Also, I'm in the process of adding extend support to a @media query

if you agree, i'll send a pull request when i'm done

Contributor

jonathanong commented Mar 14, 2013

also,

%placeholder > button
  color: black

.button
  extend: %placeholder > button

would work too

Contributor

jonathanong commented Mar 15, 2013

https://github.com/jonathanong/rework-inherit

it's done. look at the API. if you want it in core, i can merge it in. it will fix all the extend issues. or you can just reference it and remove extend from core (i can make an alias for extend, and i can give you commit privileges).

Owner

tj commented Apr 4, 2013

i'd definitely like to have at least the more common use-cases fixed, some are a bit subjective I think but pseudos etc are definitely necessary

Contributor

jonathanong commented Apr 4, 2013

which specifically? partial selectors and media queries? i can remove those features and submit a pull request if you'd like. however, i'm pretty sure someone is inevitably going to complain about those features like they do in almost every preprocessor.

Owner

tj commented Apr 4, 2013

yea without the media stuff it should be good

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