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

[jss-nested] target a rule from the same sheet when possible #214

Closed
kof opened this issue Apr 19, 2016 · 69 comments
Closed

[jss-nested] target a rule from the same sheet when possible #214

kof opened this issue Apr 19, 2016 · 69 comments
Labels
enhancement No kittens die if we don't do that.

Comments

@kof
Copy link
Member

kof commented Apr 19, 2016

Sometimes we need to create a nested selector targeted to a child class from the same sheet, for e.g. when we want to change a style for some inner element.

In this case, button is a global selector scoped to the container selector

{
  container:  {
    '&:hover button': {
      color: 'red'
    }
  }
}

Results in something like this:

.container-jss-0-0:hover button {
  color: red;
}

Now we want to target an element with generated class name:

{
 button: {
   color: 'blue' 
 },
  container:  {
    '&:hover button': {
      color: 'red'
    }
  }
}

Now we checked that button is defined on the same sheet, so lets use the generated for it selector.

.container-jss-0-1:hover .button-jss-0-0 {
  color: red;
}
@kof
Copy link
Member Author

kof commented Jun 7, 2016

@fgnass
Copy link

fgnass commented Jun 7, 2016

Cool, this is very similar to what I first had in mind for https://github.com/fgnass/jss-local-refs. One thing that felt not very intuitive from a user's perspective was the fact that button needs to be defined before container. It also felt strange (and maybe error-prone) to use button which looks like it was referring to a tag name.

Another option I considered was &:hover jss(button) to address the latter issue, but it felt equally awkward.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

not very intuitive from a user's perspective was the fact that button needs to be defined before

yep, we could find rules which refer to other rules, remove them and add them again, this way they will be able to reference rules defined after.

Another option for refs would be $button: pro - it is more explicit, less magic, contra - more ugliness

@kof
Copy link
Member Author

kof commented Jun 7, 2016

I still tend to use an unprefixed ref, because we are in a namespaced sheet, which means all keys are just names, they are never selectors.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

The only prob one can get if he has already a button rule and now actually wanted to use button as a selector, same is for .# ... they are all global selectors.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

On the other hand why should someone have a button rule and then to try reference button as a selector. This tends to be already broken ... it should not happen ....

@kof
Copy link
Member Author

kof commented Jun 7, 2016

another option would be this.button

@kof
Copy link
Member Author

kof commented Jun 7, 2016

another option @button

@kof
Copy link
Member Author

kof commented Jun 7, 2016

anothoer option ..button

@kof
Copy link
Member Author

kof commented Jun 7, 2016

Another option &&button ... & means actually "parent", && would mean the rules set, because it is a parent of a rule (see http://stylus-lang.com/docs/selectors.html#parent-reference)

@fgnass
Copy link

fgnass commented Jun 7, 2016

we could find rules which refer to other rules, remove them and add them again, this way they will be able to reference rules defined after.

Ah, that's clever! I was looking for some sort of event to listen to, but this sounds much easier.

@fgnass
Copy link

fgnass commented Jun 7, 2016

What about &:hover .button (it's a class after all) and &:hover global(.someAlienClassName) to opt into global selectors?

@kof
Copy link
Member Author

kof commented Jun 7, 2016

What about &:hover .button (it's a class after all) and &:hover global(.someAlienClassName) to opt into global selectors?

If we use global() then we don't need any prefixes at all! yay I like it. As a bonus, global selectors become very explicit. The only bad thing is that it is a breaking change.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

So what if we do &:hover global(.any-global-selector) and &:hover localRuleName ?

@kof
Copy link
Member Author

kof commented Jun 7, 2016

Oh nice we can make it even backwards compatible: if we don't find a rule locally, we can always assume it is a global selector, so peoples code won't break.

@fgnass
Copy link

fgnass commented Jun 7, 2016

I would prefer to have early errors instead of falling back to global selectors. That's also the reason why I'd prefer &:hover .button over &:hover button – the former one would allow us to raise an early error if no such local class name exists.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

Maybe instead of errors - warnings?

@kof
Copy link
Member Author

kof commented Jun 7, 2016

This way we can stay backwards compatible and also advocate using explicitly global()

@kof
Copy link
Member Author

kof commented Jun 7, 2016

CSS modules is using :global() ... that would be fine too, for the sake of similarity
https://github.com/css-modules/css-modules#exceptions

@fgnass
Copy link

fgnass commented Jun 7, 2016

If we implement this local/global feature as separate plugin the jss-nested codebase could stay as it is (no breaking changes there). On the other hand I wouldn't mind a major version upgrade that introduces this new local/global handling and breaks old stuff.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

Can you imagine any other use cases for :global() other than within jss-nested?

@kof
Copy link
Member Author

kof commented Jun 7, 2016

If one needs the entire sheet being global, there is an option named. Which becomes right now not very accurate and should be renamed to global.

@fgnass
Copy link

fgnass commented Jun 7, 2016

You're right, most (if not all) use cases should be related to nested selectors. I guess I just like JSS' modularity as it allows for incremental refinements. But I also understand the wish to not have too many plugins.

@kof
Copy link
Member Author

kof commented Jun 7, 2016

We can split it once we see a use case, right now, I can't find any. Also in case you are interested see #241

@fgnass
Copy link

fgnass commented Jun 7, 2016

I just tried to add support for the following:

{
  container: {
    '&:hover button': {
    }
  },
  button: {
  }
}

The problem I have is that when the plugin comes across the nested selector and calls createRule() the button rule has not yet been created. I could use setImmediate but I'd rather keep things synchronous. A possible solution would be to refactor https://github.com/jsstyles/jss/blob/master/src/StyleSheet.js#L37 in a way that all the rules get registered first and the plugins are run afterwards in a second loop. Can you think of a better approach?

@kof
Copy link
Member Author

kof commented Jun 8, 2016

Yep, this is exactly what we need also for this issue #180

@kof
Copy link
Member Author

kof commented Jun 10, 2016

How do you like this:

{
  container: {
    padding: 20,
    '&': {
      ':hover': {
        background: 'blue'
      },
      'global(button)': { // .container-jss-0-0 button
        background: 'red'
      }
      button: { // .container-jss-0-0 .button-jss-0-1
        background: 'green'
      }
    } 
  },
  button: {

  }
}

@kof
Copy link
Member Author

kof commented Jun 10, 2016

I still don't really like global(button) part in the above example, it is unnatural for js and requires parsing.

@fgnass
Copy link

fgnass commented Jun 10, 2016

What about the following to get rid of global()?

{
  container: {
    padding: 20,
    '&': {
      ':hover': {
        background: 'blue'
      },
      button: { // .container-jss-0-0 .button-jss-0-1
        background: 'green'
      }
    },
    '& .someGlobalClass' { // .container-jss-0-0 .someGlobalClass
    }
  },
  button: {
  }
}

@kof
Copy link
Member Author

kof commented Jun 10, 2016

Oh thats actually interesting, it can be even backwards compatible.

@fgnass
Copy link

fgnass commented Jun 10, 2016

The following would work too. One just has to create new &-block to target tag names:

{
  container: {
    padding: 20,
    '&': {
      ':hover': {
        background: 'blue'
      },
      '.someGlobalClass' { // .container-jss-0-0 .someGlobalClass
      }
      button: { // .container-jss-0-0 .button-jss-0-1
        background: 'green'
      },
    },
    '& button' { // .container-jss-0-0 button
    },
  },
  button: {
  }
}

@kof
Copy link
Member Author

kof commented Jun 10, 2016

So basically it can be

'&:hover': {...rule}
'& .global-selector': {...rule}
'&': { localRuleName: {...rule} }
'&:hover': { localRuleName: {...rule} }
and even
'& .global-selector': { localRuleName: {...rule} }

@kof
Copy link
Member Author

kof commented Jun 10, 2016

The only problem in my example is to differentiate a rule object from styles object.

@kof
Copy link
Member Author

kof commented Jun 10, 2016

Ok considered values will be soon also objects, we need to make a restriction: key should be just & and doesn't allow selectors after & if children should be considered as local .

@kof
Copy link
Member Author

kof commented Jun 10, 2016

Now, what if we just leave the way it works right now as it is and just add the single &, which can handle local refs only :

{
  container: {
    padding: 20,
    '&': {
      button: { // .container-jss-0-0 .button-jss-0-1
        background: 'green'
      }
    }
  },
  button: {
     ...
  }
}

The rule would be:

  1. Everything after & is global: &:hover, & .class &.class &#id & tag. & just gets replaced by a parent selector, as it is now.
  2. If single & is used, values are rules, where key is always a local rule ref.

@kof
Copy link
Member Author

kof commented Jun 10, 2016

Seems to me like it would be very easy to implement and fast. No string parsing. Pretty much explicit. No breaking changes.

@avocadowastaken
Copy link
Contributor

May be we should add some variable prefix sign,

$ sign as in SASS

{
  container: {
    padding: 20,
    '$button': { // .container-jss-0-0 .button-jss-0-1
        background: 'green'
     }
  },
  button: {
  },
  `$button`: { // will throw `Unexpected $ token` error
  }
}

@kof
Copy link
Member Author

kof commented Jun 20, 2016

Mentioned that already too.
Pros:

  • easier to parse than something with balanced parenthesis

Contras:

  • not really js like, we have real variables and objects in js
  • not really nice, reminds me of php)))

@avocadowastaken
Copy link
Contributor

@kof

May be we should just change our point of view, I mean that rule should change only it's target, and it's target can not be changed from outside

{
  container: { 
    padding: 20, 
  },
  button: {
    color: 'red',
    'container &': {
      color: 'blue'
    },
    'container div &': {
      color: 'blue'
    }
  }
}

But here we can have problems if rule name will be same as html tag

{
  section: { // .section-jss-0-0
    padding: 20, 
  },

  button: {  // .button-jss-0-0
    color: 'red',
    'section &': { // .section-jss-0-0 .button-jss-0-0 
      color: 'blue'
    },
    'section div &': { // .section-jss-0-0 div .button-jss-0-0
      color: 'blue'
    }
  }
}

You wan't be able to make selector like - section .button-jss-0-0 in this case

@fgnass
Copy link

fgnass commented Jun 20, 2016

I think I like that idea. We probably need some sort of naming scheme here and $button (which would not even need to be quoted) makes it quite clear that this is a reference to something.

@kof
Copy link
Member Author

kof commented Jun 23, 2016

I totally agree that $button is clear from syntax perspective, but at the same time it is still a string based way, I feel like using objects and functions is a better way to go for jss.

@kof
Copy link
Member Author

kof commented Jul 13, 2016

Lets use "@button" ... it is basically like $button, but more css like.

@fgnass
Copy link

fgnass commented Jul 13, 2016

Isn't that a problem? I mean how shall we disambiguate this from "@media", "@import", "@keyframes" or any possible future addition to the CSS spec?

@avocadowastaken
Copy link
Contributor

@kof 👍

@fgnass

it's easy to validate like https://github.com/umidbekkarimov/jss-responsive/blob/master/src/jss-responsive.js#L95 and throw reserved variable name exception.

Like in js you can not define variable delete, package and else

@kof
Copy link
Member Author

kof commented Jul 13, 2016

@fgnass I think we need only rules of rule.type === 'regular', right?

@fgnass
Copy link

fgnass commented Jul 13, 2016

Imagine I created a media object, call my local class "media" and then later on want to reference it in a nested rule. I'd argue that the error message about using a reserved name would be super confusing, highly inconvenient and totally unexpected.

If I as developer for example spotted @element somewhere in a JSS context, I'd assume it's about element queries, not an alias for .button--jss-0 .

@fgnass
Copy link

fgnass commented Jul 13, 2016

@kof yes, I'd say so

@kof
Copy link
Member Author

kof commented Jul 13, 2016

Good point about element query, lets see how it could be implemented in the future to make sure it won't be a problem - #267

@yury-dymov
Copy link

I extended implementation of jss-local-refs: https://github.com/yury-dymov/jss-local-refs

Waiting PRs to be merged

  1. Implemented integration with jssNested plugin, more details here: implementation for jssNested case fgnass/jss-local-refs#1
  2. Changed exception to warning in case class is missing. We still would like to support usage of global classes like 'active' for example.

The thing, which bothers me though is that we still have to arrange classes in the certain order:

{
 a: {},
 b: {
   '& > .a': {}
 }
}

will work fine

{
 b: {
   '& > .a': {}
 },
 a: {}
}

will not work.

But after consideration I came to conclusion that it might be not a bug but a feature. In the second example global 'a' class will be used instead of the local one, which on the one hand might solve some issues discussed above but on the other - makes supporting the code harder, which is not a good thing.

@yury-dymov
Copy link

update: jss-refs exists already, I reinvented the wheel :)

@kof
Copy link
Member Author

kof commented Jul 20, 2016

ok @fgnass lets go for $ and finally make the final effort in this long issue!

@kof
Copy link
Member Author

kof commented Jul 31, 2016

@fgnass I had a sleepless night and decided to implement the last suggestion, basically I just ported your https://github.com/fgnass/jss-local-refs.

Thanks a lot for helping out! I will close this issue as it is released, please let me know if there are any issues with that.

@kof kof closed this as completed Jul 31, 2016
@msuntharesan
Copy link

msuntharesan commented Oct 25, 2016

the $ref doesn't work when its inverted like $container > &.

@kof
Copy link
Member Author

kof commented Oct 25, 2016

@VanthiyaThevan currently the & needs to be first character, but this can be chanchged, because it doesn't allow some other cases. Unrelated issue though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement No kittens die if we don't do that.
Projects
None yet
Development

No branches or pull requests

5 participants