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

Restrictions on namespaces and mixins names #1205

Closed
SomMeri opened this issue Feb 28, 2013 · 10 comments
Closed

Restrictions on namespaces and mixins names #1205

SomMeri opened this issue Feb 28, 2013 · 10 comments

Comments

@SomMeri
Copy link
Member

SomMeri commented Feb 28, 2013

What is officially acceptable namespace/mixin name? I'm asking because I could not come up with consistent set of rules. I made some observations, but I'm not sure what is how it is supposed to, what is incidental be behavior and whether something is a bug.

The most suspicious point is 4b.

1.) The distribution of and> in mixin call does not seem to matter. All following mixin calls seems to match exactly same set of mixins:

  • .amp.ext.support;
  • .amp .ext.support;
  • .amp.ext .support;
  • .amp .ext .support;
  • .amp > .ext.support;
  • .amp.ext > .support;
  • .amp > .ext > .support;

2.) Mixins name can be composed of multiple classes/ids. Selector combinators and ampersands between classes/ids does not matter. All above mixin calls match all these declarations:

.amp.ext.support {
  declaration: other; //all hit this
}
//inside namespace
.amp {
  &.ext.support {
   matched: amp extended; //all hit this
  }
  + .ext.support {
   color: plus extended; //all hit this
  }
  .ext.support {
   matched: extended; //all hit this
  }
  &.ext .support {
   matched: amp extended with space; //all hit this
  }
  .ext + .support {
   matched: extended with plus; //all hit this
  }
  .ext {
    .support {
   matched: nested; //all hit this
    }
  }
}

3.) Spaces inside names does matter, this will not be matched:

.amp {
  &.ext.sup port {
    no-match: space in id or class; //no match
  }
}

4a.) Extensions of namespaces names are ignored. Neither of these will be matched:

.amp.ext {
  &.support {
    color: first blue; //NONE hit this
  }
}
.amp .ext {
  &.support {
    color: second blue; //NONE hit this
  }
}

4b.) Extensions of namespaces names are ignored. Both of these will be matched by all above mixin calls:

.amp.ext {
  .ext {
    .support {
      testing-scope: should not match 1; //all hit this
    }
  }
}
.amp .ext {
  .ext {
    .support {
      testing-scope: should not match 2; //all hit this
    }
  }
}

I placed the whole case into a gist.

@lukeapage
Copy link
Member

probably another case of incomplete code that has not caused a problem - just an inconsistancy

  1. surprising.. but I guess people are highly likely to rely on this and not be happy if we break it :(
  2. initially surprising, but then I remembered how the code works (ignores combinators and just compares elements)
    2a - if we make it max exactly - people may not be able to get properties when they could before
    2b - we keep it as its is - it makes it impossible to distinguish different selectors
    tempted to leave this as it is unless it is causing issues for people.
  3. I would expect this and I think this is good/as expected
  4. a.

none match this either

.amp.ext {
  &.support {

but this does match

.amp { 
  &.ext.support {

so it has nothing to do with ampersand support.

that looks v. suspicious to me

  1. b. I agree, definitely a bug

@SomMeri
Copy link
Member Author

SomMeri commented Mar 6, 2013

Thank you for detailed answer.

I do not like one particular aspect of 1. It was surprising to find out that these two are equivalent:

  • #other > .mixin;
  • #other.mixin; (no space here)

It was very unexpected, at least for me, that this call:

#something > .mixin;

matches this:

  #other.mixin {
    //mixin definition
  }

These two being equivalent is perfectly nice and concise

  • #namespace1 > #other > .mixin;
  • #namespace1 #other .mixin;

What bothers me is this one:

  • #namespace1#other.mixin; (no spaces)

@SomMeri
Copy link
Member Author

SomMeri commented Mar 6, 2013

I think that optional space between namespace and mixin would be ok, it less would not allow extended mixin names. But it does allow them (point 2) and they are nice feature I think. It has both extended mixin names and optional space, so there is no way to distinguish between these two mixins:

Mixin 1:

  #other.mixin {
    //mixin definition
  }

Mixin 2:

  #other {
   .mixin {
      //mixin definition
   }
 }

@SomMeri
Copy link
Member Author

SomMeri commented Aug 30, 2013

Another observation: only mixins names can contain combiner, namespaces can not.

Following does not work:

.class > .child {
  .inside {
    inside: inside;
  }
}

#inside-long {
  .class > .child > .inside;
}

It is ok I guess, but I wanted to collect the whole behavior on one place so it can be easily documented later on.

This was referenced Oct 28, 2013
@seven-phases-max
Copy link
Member

To keep things up-to-date, the above patch fixes the following examples here:

@seven-phases-max
Copy link
Member

I wonder if we actually consider (1) and (2) as a bug. It's just a historical Less feature and I see no way to "fix" it w/o major breaking changes.

(3) is an intended behaviour.

Thus considering (4) is fixed this issue has some chances to be closed :)

@SomMeri
Copy link
Member Author

SomMeri commented Sep 6, 2014

@seven-phases-max You are mostly right, but the property described in this comment is still bothering me. If we think we can not change it due to backward compatibility, then so be it. But if we can, I would like to fix it.

@seven-phases-max
Copy link
Member

@SomMeri

I'd rather put it in a way of "do we really need to change it?".

The trick is that when we actually use this syntax to invoke #namespace/.mixin thing which is written and indented to be used as a mixin (i.e. "standard" #ns { .mixin {...} } definition) then whitespaces do not matter.
And if we try to use this syntax to invoke a CSS selector(s) where whitespace does matter (which in fact automatically means that styles of this selector are not meant to be used as a mixin) it will most likely fail anyway (except the very basic cases) regardless of actual syntax and whitespaces (by "fail" I mean it will compile but not into something a user would expect).
This last sentence may look surprising and it's definitely not evident and I would like to write a detailed explanation to support it but I'm afraid this takes forever (honestly I tried).
So probably this collection of the SO questions and answers can give a basic idea of what's wrong with "using CSS classes as mixins" in general (and thus why "whitespace issue is the least significant problem there"). Those examples are quite one-sided (as they are all about Bootstrap and the way it's written), but the same should apply (less or more) to any other framework and/or whatever complex Less/CSS code-base. So to support it more I would mention that I can't recall even a single SO question where "whitespace issue" was the show-stopper (and btw. not so many (1?) of such are here at less/less.js).

So I'd say that while with no doubt this remains an issue and definitely looks like a significant one in theory, in practice it turns out to be no more than just a tiny bogus :)


But regardless of above now I think you're right and we're better to keep it open (maybe I would just decrease its priority), in fact I would love to see ideas of possible solutions (because I suspect that an attempt to fix this in an "extend way" will introduce even more problems, but you never know... :).

@SomMeri
Copy link
Member Author

SomMeri commented Sep 8, 2014

What I took from what you wrote is that (nothing) is just multiple classes css combinator, just like is child or~ is sibling. Since other combinators are treated the same, this one should be no exception. So, basically you think about it in terms of html and generated css.

I can buy the above view of things. I was thinking about it in terms of how less looks like and current behavior made little sense, now it makes sense to me. So, now I think it might be better to close this, unless we can clearly state what the problem is.

@seven-phases-max
Copy link
Member

So, basically you think about it in terms of html and generated css.

:) Interesting, for me it always looked like I do the opposite unless implementation details and backward compatibiity dictate something too hard (that's what >> example was about)
(Btw., I personaly use .ns.mixin() syntax instead of #ns > .mixin() and this way it does not seem like too tied with HTML and CSS :). But never mind, obviously my main point was that "I cant see how we can fix whitespace w/o breaking it all to fix a subtle problem" but looks like I failed again :)
I'm happy to know that at least something of it made sense to you, thanks for reading!

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

No branches or pull requests

3 participants