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

Support conditional rule groups for named styles #46

Closed
kof opened this issue Feb 9, 2015 · 37 comments
Closed

Support conditional rule groups for named styles #46

kof opened this issue Feb 9, 2015 · 37 comments

Comments

@kof
Copy link
Member

kof commented Feb 9, 2015

As a result of #45

We need to support conditional rule groups like @media in named style sheet https://developer.mozilla.org/de/docs/Web/CSS/At-rule

One proposal would be to write it like this

{
    myName: {
        condition: '@media (min-width: 1024px)',
        color: 'blue'
    }
}

Another proposal

{
    button1: {
        color: 'red'
    },
    '@media screen and (min-width: 768px)': {
        button1: {
            color: 'green'
        },
        button2: {
            color: 'blue'
        }
    }
}

@kof kof added the task label Feb 9, 2015
@kof
Copy link
Member Author

kof commented Feb 9, 2015

The reason for a different syntax is that we need all rules to be accessible via sheet.getRule and sheet.classes.myRule ... because thats the point of named style sheets.

@markdalgleish
Copy link
Contributor

FWIW, on my initial reading of the docs, I got the impression that you could use media queries with namespaces based on the first JSS syntax example.

@markdalgleish
Copy link
Contributor

Whether that means the API is confusing, the readme is confusing, or I'm just generally confused due to my own issues, I don't really know.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

yep, this example was initially without using named style sheet. After I changed named style sheet to be default and changed the example, @media part of it became invalid, thats why I have removed it for now until we implement it right.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

So confusion is because of this wrong example.

@markdalgleish
Copy link
Contributor

My completely uninformed assumption is that you should be able to write what basically looks like CSS and it should just work, even if you used namespaces.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

Ideally yes, but in the real world namespaces mean we need to have a common scope for them. In case of conditional group rules they kinda introduce a new scope. So we need to break CSS compatibility when using named styles in this case to have just one scope per sheet.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

shouldn't be a big issue though, because there are not much conditional group rules, its just @media and @supports which is probably not used.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

another possibility would be to allow conditional group rules per style sheet.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

So that all rules within one style sheet are within @media condition.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

this would make 100% consistency with css but will force you to write separate sheets for this. Maybe not the worst option.

@markdalgleish
Copy link
Contributor

The point I seem to have missed is that you can't override a namespace like you can with a selector when doing responsive design. Is this correct?

@kof
Copy link
Member Author

kof commented Feb 9, 2015

Correct right now. But what if we allow this? And treat all declarations within a conditional group rule as an override if such a rule exist, if not - it will be added? May be also not bad at all.

@markdalgleish
Copy link
Contributor

Makes a lot of sense to me. IMO the principle of least surprise would suggest you should be able to override just like normal CSS, otherwise this is just going to trip more people up.

@markdalgleish
Copy link
Contributor

I assumed that namespaces just meant that JSS would generate class names for you, but otherwise allow you to just write CSS with a JSON syntax, at-rules and all.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

there are 2 basic ideas:

  1. as you said - generated classes
  2. access to the rules from javascript

The last point could become less intuitive if we start overriding rules, but maybe its not that important there.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

In terms of implementation details: a rule can become a container rule with subrules which have conditions. But at the end it should render to css we expect.

@markdalgleish
Copy link
Contributor

On the question of accessing the rules from JavaScript, maybe it would look something like this:

sheet.getRule('myName');

//returns:
{
  color: 'red',
  '@media screen and (min-width: 768px)': {
    color: 'green'
  },
  '@media screen and (min-width: 1024px)': {
    color: 'blue'
  }
}

That would be the least surprising to me. Would that cause any issues? What are your thoughts on this API?

@kof
Copy link
Member Author

kof commented Feb 9, 2015

this structure looks strange because @media conditional is on the same level as property names.

@kof
Copy link
Member Author

kof commented Feb 9, 2015

getRule returns a Rule instance anyways ... I was thinking forward about getRule('...').toJSON()

@kof
Copy link
Member Author

kof commented Feb 9, 2015

maybe toJSON should have an option for returning declarations only or with conditionals.

@kof
Copy link
Member Author

kof commented Feb 15, 2015

I think this is the best syntax.

{
    button1: {
        color: 'red'
    },
    '@media screen and (min-width: 768px)': {
        button1: {
            color: 'green'
        },
        button2{
            color: 'blue'
        }
    }
}

Will try to implement something.

@kof
Copy link
Member Author

kof commented Feb 15, 2015

Hmm another issue with this syntax is that its numeric values can't be easily constants, by nature of object properties.

@kof
Copy link
Member Author

kof commented Feb 15, 2015

Is this actually an issue?

var SOME_CONSTANT =  768

var rules = {
    button1: {
        color: 'red'
    }
}

rules['@media screen and (min-width: '+ SOME_CONSTANT+'px)'] = {
        button1: {
            color: 'green'
        },
        button2: {
            color: 'blue'
        }
}

@markdalgleish
Copy link
Contributor

I can definitely see that becoming a bit annoying.

However, I still really appreciate the fact that it's a totally guessable format. If asked to use JSON to define CSS + media queries, I'm sure most people would produce something similar.

Plus you could easily build some helpers to generate this format for you, if you feel the need.

@0m15
Copy link

0m15 commented Feb 16, 2015

Also, I think that it should be possible to extend rules with an @-rule in it.
What about this:

var mixin = {
    '@media (min-width: 480px)': {
        display: 'block'    
    }
}

var rule = {
    extend: mixin
    background: '#ccc'
}

Would be achievable? It's something that has more to do with extend plugin, or it's better to discuss it here?

@kof
Copy link
Member Author

kof commented Feb 16, 2015

@Zimok lets talk about it separately after we have a good working @media solution.

@0m15
Copy link

0m15 commented Feb 16, 2015

@kof yes, it makes sense

By the way, talking about @-selector rules, I believe in this case a special prop keyword, like condition, would not be that weird, even if it's something completely unknown in css world – but, otherwise, what would be the benefit of using a pre-processor, if we can't keep things small and DRY?

Plus it would make the code more readable and smaller, avoiding repetitions (compare it to the alternative solution where you have to repeat the name of the rule every time within an @-rule).

I'd definitely prefer a special keyword against an over-nested, repeating structure, but that's a matter of personal style.

Eventually, I think we should also find a solution for using @-rules with named stylesheets.

@art-in
Copy link

art-in commented Sep 3, 2015

@kof How things going with this?

I would really appreciate @media support in such form
(your second proposal for API looks prettier for me btw.)

I know with 'jss' we can create several stylesheets with different media queries.

jss.createStyleSheet({_rules1_}, {media:'_query1_'}}).attach()
jss.createStyleSheet({_rules2_}, {media:'_query2_'}}).attach()

But I'm using 'react-jss' and we can create component with one stylesheet only, right?

useSheet(ReactComponent, {_rules_}, {media:'_query_'})

@kof
Copy link
Member Author

kof commented Sep 4, 2015

I am on it now, should be ready next week.

@kof
Copy link
Member Author

kof commented Sep 6, 2015

its finally landed, please try it out, would love your feedback

@art-in
Copy link

art-in commented Sep 6, 2015

@kof Great! JSS works as expected.

But I have a question regarding to 'react-jss'.
I have following jss definition:

  css: {
    externalRule: {
      'border': '1px solid red'
    },
    '@media print': {
      externalRule: {
        'border': '5px dashed black'
      },
      subRule: {
        'font-weight': 'bold'
      }
    }
  }

And in this.props.sheet.classes I got:
{externalRule: "jss-0-2", @media print: undefined}

My expectation is:
{externalRule: "jss-0-2", subRule: "jss-x-x"}
I.e. do not have @media and do have subRule there.

I guess this happens because 'react-jss' do not know about subrules yet.
What do you think? Should I forward this to 'react-jss' repo as an issue?

@kof
Copy link
Member Author

kof commented Sep 6, 2015

thanks for the report,

  1. removed @media from classes map
  2. subrules from @media are designed as overwrites, if you want to access it via classes, define it in the main object

@art-in
Copy link

art-in commented Sep 6, 2015

@kof yep, do not see @media in classes anymore.
Subrules overwrite behavior is not something obvious imho, but I do not see cases where it is going to be a blocking issue, though user always can add empty subrule at the top level to get it work...
Thanks!

@kof
Copy link
Member Author

kof commented Sep 6, 2015

Yep, lets see if someone will stumble over it.

@kof
Copy link
Member Author

kof commented Sep 6, 2015

closing for now, if something comes up feel free to reopen.

@kof kof closed this as completed Sep 6, 2015
@kof
Copy link
Member Author

kof commented Sep 10, 2015

Look whats possible with ES6

const WIDTH = 700
{
    button: {
        color: 'red'
    },
   [ `@media screen and (min-width: ${WIDTH}px)`]: {
        button: {
            color: 'green'
        }
    }
}

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

4 participants