cssinjs / jss

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

Using arrow function on jss-rtl dependent property does not works well #1234

Open sashberd opened 4 years ago

sashberd commented 4 years ago

Expected behavior:

The rtl plugin should switch the styles.

Describe the bug:

The rtl plugin doesn't switch all the styles.

const useStyles = createUseStyles({
  myButton1: {
    paddingLeft: "16px" ✅
  },
  myButton2: {
    paddingRight: ({ isMobile }) => (isMobile ? 0 : "26px") ❌
  },
  myLabel: props => ({
    paddingRight: "8px" ✅
  })
});

Codesandbox link:

https://codesandbox.io/s/react-jss-playground-37lqs

Versions (please complete the following information):

First reported in https://github.com/mui-org/material-ui/issues/18477.

oliviertassinari commented 4 years ago

@sashberd Please provide a reproduction not including Material-UI.

sashberd commented 4 years ago

Here you can see that paddingRight is not calculated at all https://codesandbox.io/s/react-jss-playground-q1bbc

oliviertassinari commented 4 years ago

@sashberd Thanks! I have updated the codesansbox to use the latest version of all the dependencies: https://codesandbox.io/s/react-jss-playground-37lqs and to better illustrate the issue with 2 buttons.

sashberd commented 4 years ago

I found another similar issue:

If you do next:

 textFieldLabelRoot: ({ fontSize = '16px',  fontColor = axisPalette.accentTextColor })=> ({
    '&$focused': {
      color: axisPalette.accentColor,
    },
    '&$error': {
      color: axisPalette.inputGreyedOut,
    },
    fontSize: fontSize,
    right: 0,
    color:  fontColor,
  }),
  error: {},
  focused: {},

You will get next message in console Warning: [JSS] Could not find the referenced rule "focused" in "makeStyles". Warning: [JSS] Could not find the referenced rule "error" in "makeStyles". And those rules with &$focused and &$error will not work However, if I will use:

textFieldLabelRoot: {
    '&$focused': {
      color: axisPalette.accentColor,
    },
    '&$error': {
      color: axisPalette.inputGreyedOut,
    },
    fontSize: ({ fontSize = '16px' }) => fontSize,
    right: 0,
    color: ({ fontColor = axisPalette.accentTextColor }) => fontColor,
  },
  error: {},
  focused: {},

All works perfect. Maybe this is different issue, but looks like from same sector

oliviertassinari commented 4 years ago

This reference problem already has a dedicated issue.

sashberd commented 4 years ago

@oliviertassinari Can you provide the reference to it?

oliviertassinari commented 4 years ago

@sashberd It's specific to Material-UI, JSS isn't impacted. Its related to the usage of two style sheets instead of one. You should find reference of this in https://github.com/mui-org/material-ui/labels/package%3A%20styles.

kof commented 4 years ago

This is an issue with jss-rtl plugin, I assume it needs to support function values. I don't think it worked ever before.

I think this issue should be moved to jss-rtl repository

guilleasz commented 4 years ago

Are there any updates on how to solve this issue?

ccpu commented 4 years ago

@kof, as you have mentioned the problem is that the plugin is not supporting the function values, however, I have also found that it's not possible to implement the function value because of this line that removes the function from the style, hence the next plugin has no access to the functions. Also because timestamp has been used for the function name that generated by jss-plugin-rule-value-function, it is not possible to access the function unless iterate coming object to find the prop that starts with fnValuesNs or fnRuleNs, which is a hacky way to do this. I think this can also cause a problem for other plugins that required to implement the function values, hence it worth to look into it.

For now, here is the working version (hacky version) of 'jss-rtl' that renamed to jss-rtl-mui:

jss-rtl-mui npm

i would appreciate it if you could give me some advice.