cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.06k stars 397 forks source link

[jss-nested] Classes can't be defined in a nested scope? #635

Closed carlgunderson closed 6 years ago

carlgunderson commented 6 years ago

Looking at this example: https://github.com/cssinjs/jss-nested#use-rulename-to-reference-a-local-rule-within-the-same-style-sheet

Why does a class need to be defined at the root level in order to be usable in nesting? Is there a technical JSS reason for needing this structure? In the example, say the only button you have is inside .container, and you don't need/want it to have any styles outside that scope. Obviously just adding something like button: {} at the root fixes the issue. I'm just wondering if I'm missing something, because this is a common usage pattern with CSS preprocessors that support nesting.

kof commented 6 years ago

It would be easier to understand if you use examples.

So your question is why do we need:

{
  button: {
    '& span': {color: red}
  }
}

instead of

{
  '& span': {color: red}
}

Do I understand correctly?

carlgunderson commented 6 years ago

Apologies. Here is an example:

{
  container: {
    '& containerChild': {color: red}
  }
  containerChild: {} // This needs to exist for the above to work
}

Why can't we just define containerChild where it lives, i.e. inside container?

kof commented 6 years ago

I am not sure what you mean, your example would not work

Do you mean ref syntax?

{
  container: {
    '& $containerChild': {color: 'red'}
  }
  containerChild: {} // This needs to exist for the above to work
}
kof commented 6 years ago

In this case you are referencing an existing rule, the reason is you will need some name for a classes map. In theory we could generate that rule for you from the nested rule, but that might be less explicit when you use classes.containerChild. Also this makes sure you can't have the same name twice.

kof commented 6 years ago

Ideally you should not need this at all, most of the time I just use first level rules.

kof commented 6 years ago

Also containerChild shouldn't be empty, normally you have some base styles to declare in the first place, no?

carlgunderson commented 6 years ago

Alright, thanks for clarifying. It's just a difference in how we are used to utilizing nesting with CSS preprocessors. I like to avoid having all rules defined at the "first level", or root. One reason I like the nesting feature of Less/SASS is because the nested classes will visually line up with the HTML structure that they're being applied to, so you can get a sense for where your styles are getting applied and how specific they are.

kof commented 6 years ago

I like to avoid having all rules defined at the "first level", or root

In cssinjs we are trying to flatten the rules as far as possible. We have no class names collisions here, so we have no reason to nest.

because the nested classes will visually line up with the HTML structure

This is harder to maintain, because html structure might change and ideally it has less dependencies to CSS.

Also in cssinjs you should write small components, which removes all issues with readability of css if you write styles object per component. Ideally inside of the component file.

Having small components and small styles objects removes any need for additional structures in styles.

carlgunderson commented 6 years ago

You're absolutely right! I have written a lot of cssinjs lately and that has been the case for me across the board. I opened this issue mostly because I was curious and it seemed like something that should have been possible, but after talking through the implementation it definitely makes sense why it's an anti-pattern with this approach.

ilyadoroshin commented 6 years ago

we're using & .containerChild all over the place in sass, wouldn't it be better to reuse existing workflows? to make transition from sass even more easy? :)

kof commented 6 years ago

sass doesn't have automated namespace genaration, .containerChild is global class name, if you have a global one, it already works in jss as you wrote, if you want to reference a local one, convention is to use & $rulename

rob2d commented 6 years ago

I know I am commenting on a closed comment here, but had a very related question (into the ether or for anyone still breathing and willing to discuss).

So I can understand that we want to ideally flatten things but just was wondering (mostly out of curiosity and also because I want to where-possible subscribe to the mantra also): the use-case where we have a nested hover and want children to react without passing explicit hover state props and listening to mouse events is pretty common on non trivial style solutions for some UI layouts. There is quite a lot of boilerplate in this case, and you end up having to break down otherwise perfectly simple functional components sometimes. Is there an alternate way to handle this logically while maintaining a flat approach? Nested classes seems to be the best way that I can actually think of in these situations.

kof commented 6 years ago

@rob2d I am not sure what boilerplate you are talking about, can you make this point clear? I def. don't recommend to use event handlers for hover and similar in the most cases.