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

Mixin duplicates selector in Bootstrap (probable less issue) #2052

Closed
hekike opened this issue Jun 10, 2014 · 6 comments
Closed

Mixin duplicates selector in Bootstrap (probable less issue) #2052

hekike opened this issue Jun 10, 2014 · 6 comments

Comments

@hekike
Copy link

hekike commented Jun 10, 2014

Issue
Wrapping/Scoping Bootstrap generates duplicate selectors with LESS.
The issue impacts negatively the third-party use of the BS.

Input:

.bs-wrap {
    @import "../bower_components/bootstrap/less/bootstrap.less";
}

Output:

.bs-wrap .bs-wrap .container:before,
.bs-wrap .bs-wrap .container:after,
.bs-wrap .bs-wrap .container-fluid:before,
.bs-wrap .bs-wrap .container-fluid:after,
.bs-wrap .bs-wrap .row:before,
.bs-wrap .bs-wrap .row:after,
.bs-wrap .bs-wrap .form-horizontal .form-group:before,
.bs-wrap .bs-wrap .form-horizontal .form-group:after,
.bs-wrap .bs-wrap .nav:before,
.bs-wrap .bs-wrap .nav:after {

Should be:

.bs-wrap .container:before,
.bs-wrap .container:after,
.bs-wrap .container-fluid:before,
.bs-wrap .container-fluid:after,
...

_Original Bootstrap issue:_
twbs/bootstrap#13779

@mdo
Copy link

mdo commented Jun 10, 2014

Apparently this only happens to our clearfix classes, which get extended all over the place in Bootstrap.

@seven-phases-max
Copy link
Member

Yes, this is known issue / expected behaviour (#1850).


The problem is that there're a few major things that make such kind of library wrapping to be very limited:

  1. Library should not use extend (since unlike mixins, extend uses "absolute" selector path instead of "relative" one).
  2. Library should not use extend all (because each wrapped class will extend another wrapped class thus producing doubled wrapper).
  3. Library should not use selector & {...} syntax (obviously this will generate wrong selector when wrapped up).
    • other not so major issues specific to Bootstrap (for instance grid classes will be generated incorrectly).

Speaking of this particular issue (i.e. (2)) the result is expected since .bs-wrap .container extends .clearfix in .bs-wrap .clearfix selector hense the double .bs-wrap.
There's not too much you can do about this since working these limitations out would require both new Less feature ("relative extend" as in #1730 (comment) for (1)) and most likely major and breaking Bootstrap structure reorganisation for (2) (since extend all cannot be "relative" by definition the only "simple" way to get such extend(.clearfix all) to work properly is to put it (and similar "mixins") into separate file that should always be imported without wrapping e.g. @import (reference) "classes-to-use-with-extend-all.less"; (also see #1177)).

For instance a quick and dirty kludge for this particular clearfix duplication problem could be:

  • remove .clearfix from utilities.less.
  • put .clearfix into new utilities-ref.less file.
  • create my-new-bootstrap.less with:
.bs-wrap {@import "bootstrap";}
@import (reference) "utilities-ref";

But I doubt this would actually make too much sense currently since the whole approach still remains broken because of (1) and (3).

@seven-phases-max
Copy link
Member

Closing as expected behaviour. (See also #1730, #1850 if you want to propose a new feature/syntax to target the use-case)

@jonschlinkert
Copy link
Contributor

@seven-phases-max you linked to a closed issue as the duplicate, which has not been resolved. is there a duplicate that is still open?

@seven-phases-max
Copy link
Member

Oops, sorry my bad, rewording the closing reason. This is expected behaviour by current extend definition so it's neither bug nor issue to keep it open (and a corresponding feature request would probably need a dedicated ticked because it has to be a new syntax). Feel free to reopen if you think it make sense to continue it here.

@mathieumg
Copy link

I imagine there is no feature request regarding this at the moment? I would also love to be table to have access to this. I am attempting to do a simple wrap and I'm caught with the same issues as described in 4. by @seven-phases-max . For now I sed-search&replace the superfluous .class in a subsequent task, but this is merely a workaround.

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

5 participants