[react-jss][jss-nested] function value doesn't work for nested levels

[HenriBeck]

From @tibbus on July 16, 2018 15:37

It worked on 8.6.0 but it doesn't work on 8.6.1 Even though I saw this : https://github.com/cssinjs/jss/issues/682

Ex. the below code works on 8.6.0, but it doesn't work anymore and a warning is displayed in the console :

Warning: [JSS] Could not find the referenced rule fieldLabel ....

export default {
  flowLayout: {
    padding: '5px'
  },
  fieldLabel: {
    background: 'blue'
  },
  layoutGroup: {
    '&.flowLayout': {
      '& $fieldLabel': {
        marginBottom: () => {
          return '500px';
        }
      }
    }
  }
};

Copied from original issue: cssinjs/react-jss#280

[HenriBeck]

From @kof on July 16, 2018 23:44

Are you sure it worked on 8.6.0? This was actually never supported, I assume something tricked you into thinking it worked, please check again?

[HenriBeck]

From @tibbus on July 17, 2018 4:30

Hey @kof , thanks for the fast reply !

I have created 2 playgrounds with react-jss :

8.6.0 here it works, div background turns 'red' : https://codesandbox.io/s/nkvvl4z7n0 8.6.1 it does NOT work, div background stays 'blue' : https://codesandbox.io/s/9jvzo7qm6w

(it worked before 8.6.0 as well, I used 8.3.1)

[HenriBeck]

From @kof on July 17, 2018 10:32

Interesting, thanks for the report! We fixed a bug which apparently broke something that I wasn't aware it worked! cc @HenriBeck

[HenriBeck]

I think the problem is with the merge classes. Because earlier we added every static class to the dynamic sheet which meant that jss-nested could resolve the reference. But now the classes are in different sheets and jss-nested can't resolve them anymore...

Really not sure if there is a solution for this with supporting both problems 🤔

[HenriBeck]

From @kof on July 17, 2018 11:59

I think we can do it within jss-nested, since we have still an access to classes.

[HenriBeck]

We don't have access to the static classes inside the dynamic sheet sadly. We would need a way to pass the classes to jss-nested but that seems like a hack. It makes sense that we would only allow jss-nested to reference the rules in the same stylesheet, this is a tradeoff from react-jss I would say.

[HenriBeck]

From @kof on July 17, 2018 19:7

I have an idea how to fix this: unrelated to this in this PR I added a classes option to the style sheet: https://github.com/cssinjs/jss/pull/757/commits/36416cc7347f045338e6e54fd88edae652b8245a#diff-29aac1ba5d08e188c14800bc889c5474R38

This allows to pass a classes object to a stylesheet with a predefined class names. So if we pass classes from the static sheet to the dynamic one, the rules which have the same name, will use that class name and the problem should get fixed automatically.

[HenriBeck]

From @kof on July 17, 2018 19:8

I wonder if I should merge that change separately, so that we can separately fix this bug.

[HenriBeck]

We would need to release jss 10 with that fix and I don't think we are quite ready for that

_{Sent with GitHawk}

[HenriBeck]

Also did you test the change with jss-nested, if jss-nested actually considers the classes🤔

_{Sent with GitHawk}

[HenriBeck]

From @kof on July 17, 2018 20:6

That change is non-breaking so its a minor release. But have to try and see if its all we need to fix it.

[HenriBeck]

Yeah, you would expect that it works but the architecture of react-jss works differently. I think the fix should be in react-jss and not jss

_{Sent with GitHawk}

[HenriBeck]

From @kof on July 17, 2018 20:14

well, the classes option I added serves actually a different point there, but it can be used to fix this. So kinda ok to use it if we have it anyways.

[HenriBeck]

In my opinion the jss-nested rule should only reference the rules in the same sheet. That's why I think it's a problem of react-jss

_{Sent with GitHawk}

[HenriBeck]

From @kof on July 17, 2018 20:47

Yes, it won't be referencing rules from another sheet, its the classes map that is passed from static sheet to dynamic sheet.

[HenriBeck]

Would this require a breaking change in the plugins api?

[HenriBeck]

From @kof on July 18, 2018 13:12

Nope

[HenriBeck]

From @michael-ecb on July 25, 2018 7:42

Hi, does it the same problem here ? I'm trying to get a value into my property: "&$checked" but does not work for me thanks

const styles = {
        checkbox: props => ({
            width:  props.width,
            height: props.height,
            "&$checked": {
                    color: props.checkedColor
              }
        }),
        checked:{}
}

[HenriBeck]

@michael-ecb it should be '& $checked'

[HenriBeck]

From @michael-ecb on July 25, 2018 8:42

thanks @HenriBeck but I 'm using material-ui v1, they need this property to be pass like this "&$checked", I also have an example :

const styles = {
        checkbox: {
            width:  props=>  props.width, <-- working
            height: props=> props.height, <-- working
            "&$checked": {
                    color:  "#000"  <-- working
                    color: props =>   props.checkedColor    <--- does not working
              }
        }),
        checked:{}
}

thanks

[HenriBeck]

From @kof on July 25, 2018 8:47

@michael-ecb you can not use 2 color properties in one object, the last one will always override the first one

[HenriBeck]

From @michael-ecb on July 25, 2018 9:0

@kof of course it was only for give you an example... I used only one, still not works...

[HenriBeck]

From @tibbus on July 25, 2018 9:3

Looks the same as in my example, Nested level + value from function call does not work in this version.

[HenriBeck]

From @kof on July 25, 2018 9:4

In that case its what the initial issue describes, we could release a 9.minor version which would fix it, if v10 takes more time, otherwise downgrade to 8.6.0

[HenriBeck]

From @michael-ecb on July 25, 2018 9:6

thanks , for me it's not working also with previous version...

[HenriBeck]

From @tibbus on July 25, 2018 9:10

Then is not this issue, I suggest to create a sandbox example similar with mine to see exactly what's not working

[HenriBeck]

From @michael-ecb on July 26, 2018 6:23

@kof does I need open a new issue ? thanks

[HenriBeck]

From @kof on July 26, 2018 9:26

Yes if you think its not the same like the current issue nore cssinjs/jss#682

[kof]

fixed as part of #682

[HenriBeck]

This isn't fixed. Same issue as cssinjs/jss#474

[kof]

How did you verify? In my opinion they are different. That other one is only in react-jss, for jss I have tests for it.

[kof]

also I added tests based on the example in the description of this issue, it works, unless I don't have the right reproduction

[kof]

Oh I think I know whats the problem, it only exists in react-jss because those are separate style sheets, yes it is similar to #474

We need a plan how to fix those, I don't see an easy way.

[kof]

Also to consider #501

[jaschaio]

Is there a workaround I could use in the meantime?

[HenriBeck]

No, it will be possible to use function values in nested rules in jss v10 once we release it. On 14. Oct 2018, 10:33 AM +0200, jaschaio notifications@github.com, wrote:

Is there a workaround I could use in the meantime? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[HenriBeck]

This should be fixed in #1048. I will add tests for it to verify it.

[kof]

@HenriBeck was it fixed?

[HenriBeck]

Yes this was fixed: https://codesandbox.io/s/sleepy-ramanujan-5gf2y