webark / ember-component-css

An Ember CLI addon which allows you to specify styles for individual components
MIT License
540 stars 115 forks source link

Parent & selector to reference component class #107

Open oligriffiths opened 8 years ago

oligriffiths commented 8 years ago

Currently, using the & parent selector it applies a class above the component class, which breaks the whole encapsulation rule:

eg:

.button {
    .active & { 
        color: blue;
    }
}

generates:

.active .component-classname .button { color: blue; }

but it would be great if it generated:

.component-classname.active .button{ color: blue }

Thoughts?

friksa commented 8 years ago

Did you try?

&.active {
    .button { 
        color: blue;
    }
}
oligriffiths commented 8 years ago

This sass:

& ul {
    .active & {
        color: red;
    }
}

&.active ul {
    color: red;
}

produces this css:

.active .post-type-photo-9364b334 ul {
  color: red; }

.post-type-photo-9364b334.active ul {
  color: red; }

However, the 1st sass version should really generate the 2nd css version, right?

topaxi commented 8 years ago

I have a branch which deals with this issue here: https://github.com/topaxi/ember-component-css/tree/postcss-scss This currently fixes only scss (stylus and less won't work yet).

I will contact ebryn next week to coordinate the steps needed to get some fixes landed.

oligriffiths commented 8 years ago

Awesome

lolmaus commented 6 years ago

Three years and this is not resolved. πŸ˜’

Sometimes a child element must change its looks depending on a modifier on a parent. For example, a menu item must be bold when its menu section is expanded.

But I have the following code style requirement: CSS rules that decorate an element should be nested under that element's own section. This requirement makes the following CSS illegal:

.mainMenu-item {
  font-weight: normal;
}

.mainMenu-section {
  &.is-expanded .mainMenu-item {
    font-weight: bold;
  }
}

The font-weight: bold rule is applied to the item, it should be written under item and not section. This is common sense.

According to my code style, the correct way of writing the above code is the following:

.mainMenu-item {
  font-weight: normal;

  .mainMenu-section.is-expanded & {
    font-weight: bold;
  }
}

Of course, I wanted to avoid repeating .mainMenu. But since this ticket has never been resolved, I was unable to use & here.

And since ember-component-css does not allow disabling namespacing selectively, I had to revert to an ugly hack:

$this: ".mainMenu";

@at-root #{$this}-item {
  font-weight: normal;

  #{$this}-section.is-expanded & {
    font-weight: bold;
  }
}

You can imagine how much pain and misery was caused by numerous @at-root #{$this} to me and my team. 😫 I've been hating ember-component-css with all my heart every time I had to convert from styleNamespace to a manually set pair of {className: "mainMenu"} and $this: ".mainMenu". Why does reasonable rule structuring has to be so hard?!

But today I found a workaround! Feast your eyes:

& {
  $this: & !global;
}

&-item {
  font-weight: normal;

  #{$this}-section.is-expanded & {
    font-weight: bold;
  }
}
webark commented 6 years ago

I've been hating ember-component-css with all my heart

@lolmaus That’s no good!! 😞

so, one thing we have batted around doing is giving the styleNamespace an actual variable name, so that things like this can be easier.

One quick question, your component styleNamespace is represented here as mainMenu, and you are wanting to do a β€œBEM” style classes by doing {{stylenNamespace}}-item in the template?

lolmaus commented 6 years ago

your component styleNamespace is represented here as mainMenu, and you are wanting to do a β€œBEM” style classes by doing {{stylenNamespace}}-item in the template?

Yes, sir.

lolmaus commented 6 years ago

so, one thing we have batted around doing is giving the styleNamespace an actual variable name, so that things like this can be easier.

An easy way to do it is to wrap the whole component Sass file with @at-root { }.

@at-root without a selector has no meaning and does not influence the CSS ouptut in any way. But it has one side-effect: it turns variables/mixins/functions defined inside into local ones. So if you do this:

@at-root {
  $base-selector: <styleNamespace>;

  <processed Sass code>
}

...then the $base-selector variable will be available to the component's Sass code without overwriting a $base-selector variable in the global scope.

The user is still able to define global variables with the !global flag.

As for mixins and functions, !global does not work for them. They will become local and can no longer be imported. My typical use case is to share component-specific mixins and functions from a separate file (e. g. mixins.scss next to style.scss) -- this will no longer work.

This can be resolved with conditional disabling of namespacing. For example, ember-component-css could ignore Sass files that start with a dash or a double underscore, etc.

Note that two levels of disabling are required: disable namespacing and disable auto-importing. The latter is available through environment.js, but that's very inconvenient. A filename prefix/postfix is much more handy.

webark commented 6 years ago

ya. I think the reason this stuff hasn't been that much of an issue for me personally, is that I don't use "BEM" style css for the vast majority of the time. So that's why I personally (or people I work with) haven't felt this particular pain. cause something like..

.item {
  font-weight: normal;

  .section.is-expanded & {
    font-weight: bold;
  }
}

is all we would do. THAT POINT ASIDE (if you want to get into a specificity or css selector style debate, I'm more then happy to on discord..!! πŸ˜‚πŸ˜Š)

But the idea would be that something like {{styleNamespace}} would be provided, so that you could do something to the effect of..

&-item {
  font-weight: normal;

  {{styleNamespace}}-section.is-expanded & {
    font-weight: bold;
  }
}

or some other equivalent thing.

@lolmaus Is that something that would interest you?

lolmaus commented 6 years ago

@webark Yes. And above I've suggested a way how it could b implemented without leaking.

webark commented 6 years ago

@lolmaus ya. The only issue with that is it's pretty explicit to just scss. The idea would be do use postcss to do a rule string replacement. Similar to what we currently do with less, scss, and cssNext.

webark commented 6 years ago

we'd do the same for sass and stylus.. but last I checked those don't have a postcss syntax processor. (and not that many people use them either).

lolmaus commented 6 years ago

I don't use "BEM" style css for the vast majority of the time. So that's why I personally (or people I work with) haven't felt this particular pain. cause something like .item, .section is all we would do.

I don't use short semantic selectors because they make this scenario very probable:

  1. Another developer applies styles directly to a .section element.
  2. All your components that use .section are defaced.
  3. The other developer does not notice because none of his code involves your components, so he/she doesn't bother to check them.
  4. You don't notice either because your project does not have visual regression testing set up.

if you want to get into a specificity or css selector style debate, I'm more then happy to on discord

Not willing to debate over it, it's up to every developer/team to decide. But I'll share my POV: I do care about specificity because unnecessarily high specificity makes it harder to override styles, and I've seen it encourage the use of !important. πŸ‘Ž

Is that something that would interest you?

Yes. But I would very much prefer a Sass-native solution. I hate how my IDE complains on invalid syntax in every friggin' style file. 😩

webark commented 6 years ago

I hate how my IDE complains on invalid syntax in every friggin' style file.

ya. I didn't think of that point before. We could do something like $styleNamespace rather then {{styleNamespace}} and get the same thing. what do you think of that idea? (and we could create similar variables fro less, sass, and stylus)

unnecessarily high specificity makes it harder to override styles, and I've seen it encourage the use of !important.

override styles, which I feel should be generally discouraged and used sparingly, has never been an issue for me or our team. So I always feel this point is a bit of a misnomer. Specificity done right can still lead to painless overriding when necessary. (especially if you use classes sparingly) The only time !important has come into the mix is due to super legacy #main type scopes that are infuriating, and should have never been implemented.

Another developer applies styles directly to a .section element.

in our modern ember-cli apps, the vast majority of styles are in components, and so this doesn't happen for us, and when it does, it's because it's intentional. like for arrow or button styles or somethings like that.

it's up to every developer/team to decide

Agreed. Just brought it up initially to give insight as to why it hasn't been a priority for me, not that a different opinion is invalid.

One of the nicest things about css, and also one of the main annoyances, is that there are so many different ways to do things, and those can be used in conjunction, and when needs or personal styles are favored.

webark commented 6 years ago

also

hate how my IDE complains on invalid syntax in every friggin' style file.

what is it currently complaining about?

lolmaus commented 6 years ago

what is it currently complaining about?

The & parent selector can't be used at top level. :suspect:

We could do something like $styleNamespace rather then {{styleNamespace}}

I'll reiterate on my thoughts regarding this:

webark commented 6 years ago

The & parent selector can't be used at top level. :suspect:

Ahh. To support rules like &-item {. Sure, that would be annoying. But wouldn't having rules that start with #{$this} cause errors also? Cause $this wouldn't be declared in the style file.

I'll reiterate on my thoughts regarding this:

Ya. I get all this. It's just... That seems like a lot to do in-order to solve the issue. I'll think more about this and get back to you.