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

[styles] makeStyles + @media + Adapting based on props: not updating correctly #20446

Closed
2 tasks done
csantos1113 opened this issue Apr 6, 2020 · 7 comments
Closed
2 tasks done
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Milestone

Comments

@csantos1113
Copy link

csantos1113 commented Apr 6, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When a property is changed, styles that are inside a @media selector are not being re-generated.

Expected Behavior 🤔

When a property is changed, all styles that are properties-dependent should be re-generated and re-applied to the component.

Steps to Reproduce 🕹

See example here: https://codesandbox.io/s/makestyles-mediaqueries-mygqi?file=/demo.js

Steps:

  1. "Swap color" button loads with background red and border/text-shadow blue
  2. Click on the "Swap color" button
  3. Background is changed to "blue" ✅
  4. Text-shadow is changed to "red" ✅
  5. Border is kept "blue" ❌

Context 🔦

I want to modify dynamically the fontSize of the input components (input, label, helper text) based on props.
I'm receiving a custom property called size that could be either Large or Small, and changing sizes of the components based on that property.
The fontSize is expected to be responsive too, so for example

size: Large =>
fontSizes
  Sm: 14px
  M:  16px
  L:  20px
  Xl: 24px

size: Small =>
fontSizes
  Sm: 12px
  M:  14px
  L:  14px
  Xl: 16px

I need to access the props at the rule level because I'm generating the @media selectors in another utility that I can't modify

Your Environment 🌎

Tech Version
Material-UI v4.9.9
React 16.13.1
Browser all
TypeScript n/a
@csantos1113
Copy link
Author

Here are other 2 versions of makeStyles I tried, all of them with different approaches:

function at the CSS property level (it works)

const useStyles = makeStyles({
  root: {
    ...clearStyles,
    background: props => props.color,
    border: "1px solid black",
    // Not the right way to target the label.
    // Just used to compare against the @media below
    // !This works!
    "& > span:first-child": {
      textShadow: props => props.color === "red" ? "2px 2px blue" : "2px 2px red",
      textTransform: "none"
    },
    // !This works!
    "@media (min-width: 20rem)": {
      border: props => props.color === "red" ? "2px solid blue" : "2px solid red"
    }
  }
});

function at the selector level (it does not work)

const useStyles = makeStyles({
  root: {
    ...clearStyles,
    // !This works!
    background: props => props.color,
    border: "1px solid black",
    // Not the right way to target the label.
    // Just used to compare against the @media below
    // !This does not!
    "& > span:first-child": props => ({
      textShadow: props.color === "red" ? "2px 2px blue" : "2px 2px red",
      textTransform: "none"
    }),
    // !This does not!
    "@media (min-width: 20rem)": props => ({
      border: props.color === "red" ? "2px solid blue" : "2px solid red"
    })
  }
});

@oliviertassinari oliviertassinari added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Apr 7, 2020
@csantos1113
Copy link
Author

MaterialUI makeStyles and ReactJSS createUseStyles seems pretty similar each other...

So for what is worth, I also tested this directly using react-jss and got the same wrong result. See: cssinjs/jss#1327

@siriwatknp
Copy link
Member

I found this case also. https://codesandbox.io/s/mutable-dawn-xyhub?file=/src/App.js

const useStyles = makeStyles({
  // root: ({ checked }) => {
  //   return {
  //     "& + span": {
  //       color: "red"
  //     },
  //     "& $text": {
  //       color: checked ? "blue" : "orange"
  //     }
  //   };
  // },
  root: {
    "& $text": {
      color: "purple"
    }
  },
  text: {}
});

The current root works, but if you remove the comment section and remove existing root, text color won't change based on props checked. This happen with function as css + $.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Nov 11, 2020
@oliviertassinari oliviertassinari changed the title makeStyles + @media + Adapting based on props: not updating correctly [styles] makeStyles + @media + Adapting based on props: not updating correctly Nov 11, 2020
@oliviertassinari
Copy link
Member

An update, this issue is being resolved in v5 thanks to #22342 and the new @material-ui/styled-engine package.
The same codesandbox but updated & working with the latest alpha version: https://codesandbox.io/s/makestyles-mediaqueries-forked-h0iq5?file=/demo.js.

@oliviertassinari oliviertassinari added this to the v5 milestone Nov 11, 2020
@mkhoussid
Copy link

@oliviertassinari Awesome!

Will this also be resolved with v5, by chance?

As a side question, why did you guys decide on emotion instead of styled-components?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2020

@mkhoussid #15511 should be solved too. You can find the reasoning for emotion over styled-components in #22342, fundamentally, we think (after studying them in-depth) that the concepts API of styled components are better implemented.

@oliviertassinari
Copy link
Member

An update, we have now made enough progress with the new @material-ui/styled-engine package in v5 to move toward a progressive removal of the @material-ui/styles package (based on JSS). The current plan:

  • In v5.0.beta.0, this package will come standalone, in complete isolation from the rest.
  • In v5.0.0, this package will be soft deprecated, not promoted in the docs, nor very actively supported.
  • During v5, work on making the migration easier to the new style API (sx prop + styled() API). We might for instance, invest in the documentation for using react-jss that has more or less the same API. We could also invest in an adapter to restore the previous API but with emotion, not JSS.
  • In v6.0.0, this package will be discontinued (withStyles/makeStyles API).

This was made possible by the awesome work of @mnajdova.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

No branches or pull requests

4 participants