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

Add failing test for 'toString' on media queries #45

Merged

Conversation

markdalgleish
Copy link
Contributor

I seem to have found a bug when calling toString on style sheets with media queries.

I'm not sure yet how to fix this, but I figured I'd open a discussion by providing a failing test.

The current failing test case shows how this:

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

is erroneously compiled into this:

.jss-0 {
  color: red;
}
.jss-1 {
  a: [object Object];
}

@kof kof added the bug It went crazy and killed everyone. label Feb 9, 2015
@kof
Copy link
Member

kof commented Feb 9, 2015

nice catch @markdalgleish will fix it asap

@kof
Copy link
Member

kof commented Feb 9, 2015

huh you got me debugging for quite some time util I figured out you have used @media query differently, it has been designed to be like

{
   name: {'@media (min-width: 1024px)': {a: {color: 'blue'}}}
}

where name is there to have this rule accessible via it.

@kof
Copy link
Member

kof commented Feb 9, 2015

wait, there is another issue.

@kof
Copy link
Member

kof commented Feb 9, 2015

I need to rethink at-rule syntax in jss ... I have tested this way

    var rule = new jss.Rule('@media print', {
        button: {
            display: 'none'
        }
    })

However in named style sheet, selector is generated. At-identifiers shouldn't be treated as selectors.

@kof
Copy link
Member

kof commented Feb 9, 2015

The problem is if we go this way

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

It is possible to have rules with same name ... So rules within @media become unaccessible if you want to do sheet.getRule('a')

@kof
Copy link
Member

kof commented Feb 9, 2015

I think ideally every rule should be still accessible via getRule and every rule should be able to be part of some @rule

@kof
Copy link
Member

kof commented Feb 9, 2015

This issue is exists only in named style sheet.

@kof
Copy link
Member

kof commented Feb 9, 2015

Semantically @media is a "conditional rule group" https://developer.mozilla.org/de/docs/Web/CSS/At-rule

Potentially we could have following syntax for conditionals:

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

@kof kof merged commit e6f6324 into cssinjs:master Feb 9, 2015
@kof
Copy link
Member

kof commented Feb 9, 2015

I have added your test but modified to unnamed style sheet 8ac23ec just to make sure this will work.

As for named style sheet I need to sleep over it. Its actually not a bug, its a missing feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It went crazy and killed everyone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants