fix(react-jss): Media query styles applied only to the first element in function

[pranshuchittora]

Corresponding issue (if exists):

Closes #1320

What would you like to add/fix?

Fixed react-jss

Todo

• Add tests if possible
• Add changelog if users should know about the change
• Add documentation

P.S. Will add tests if the preliminary changes are acceptable.
Demo (React JSS + React Hot Loader) -> https://youtu.be/utjjiwLn7lo

[kof]

I definitely need a test before I can look into the solution, there might be multiple ways to fix it.

[kof]

hey @pranshuchittora I am about to make a release, but sadly I can't merge it because it has no test

[pranshuchittora]

Pardon @kof was a bit busy with the work, will write tests for sure, by the end of this week.

[anthonygrignoux]

Hello, is there any news about that PR?

[pranshuchittora]

@kof I have added the test for the following fix.
And once again I apologize for the delay.

[kof]

I wonder if this test should be applied to both hook and hoc based implementation, just to be sure both work same way

[pranshuchittora]

Can you pls elaborate, like exactly which components are needed to be tested.

[kof]

createBasicTests() is the function that runs all the tests which use the passed function which creates a styled component. You picked the test file that only serves as an entry point to generate tests using hooks interface,

If we want to run the test with both interfaces, you need to add the test inside createBasicTests

Also it seems like you removed createBasicTests() call entirely from this file, which means you don't run all the tests using hooks interface at all now

[kof]

hey @pranshuchittora, any updates?

[pranshuchittora]

Hi @kof , It would to great if anyone can help me with the tests.

[pranshuchittora]

I apologize for blocking this. Due to some huge academic work, I won't be able to work on this in the coming days.
Hence closing this PR.
I am extremely sorry for that.

[kof]

I think it is very close to a mergeable state, lets keep it open, mb someone will make the required tests changes

[pranshuchittora]

Sounds great. Before proceeding with FTs It would be great if you can pull this PR locally and test the new changes manually.

[alee8]

Any updates on this PR? We make a lot of use of media query for responsive UI and have had to find ways to work around this issue (e.g., static object, static media query prop key).

[mattbeaudry]

@kof If i understand what you needed for the updated test correctly, then I think I have a fix, but let me know if more is needed. I made a PR on @pranshuchittora jss fork pranshuchittora#1

[mattbeaudry]

How's this look for the test updates: pranshuchittora#1

[jonathan-l-rivera]

Any updates? Really could use this fixed right now. It's blocking a project that uses dynamically set media queries. Any word?

[alee8]

Any updates? Really could use this fixed right now. It's blocking a project that uses dynamically set media queries. Any word?

Ditto on this question. Similarly impacted in a couple of projects as well.

@kof Any chance of this being escalated. @mattbeaudry offered test updates. If there are other issues, perhaps they can be listed so that this issue could be resolved.

[kof]

Hey everyone, I have other priorities and unless someone submits a fully mergeable PR or close to one where my involvement would be limited, nothing is gonna happen.

Asking about the status is not helpful. If you want to help - fix it.

[jonathan-l-rivera]

Hey everyone, I have other priorities and unless someone submits a fully mergeable PR or close to one where my involvement would be limited, nothing is gonna happen.

Asking about the status is not helpful. If you want to help - fix it.

@kof it looks like @mattbeaudry did 29 days ago. If that's not sufficient, that wasn't made clear.

[kof]

Didn't realize this was based off the branch with the fix, so it is passing the tests I suppose. Did anyone try the changes from this PR on a code base?

Those are massive and I am a bit afraid they will break stuff.

[EZEDSEA]

Didn't realize this was based off the branch with the fix, so it is passing the tests I suppose. Did anyone try the changes from this PR on a code base?

Those are massive and I am a bit afraid they will break stuff.

@kof I just pulled down the branch and tested it against our codebase. I can confirm that everything seems to work as expected and it DID fix some bugs we had that is effected by this issue.

[kof]

@EZEDSEA good to know, I am gonna test it again soon and release a .pre version for everyone to test

[kof]

Lol, those tests pass old lib code as well #1442 (comment)

[kof]

We can't merge this until we have proper tests, validating the fix.

[EZEDSEA]

We can't merge this until we have proper tests, validating the fix.

@kof I noticed you ended up merging it. Were @mattbeaudry tests sufficient in the end or do we still need to add some more tests to validate?

[kof]

I merged it because its a good test to have, but it doesn't verify the behavior of the changes made in this PR at ALL, so basically it has nothing to do with this PR. We still need tests that verify changes that are made here and fail on the code base before the changes were made.

[alee8]

Summary of Current Situation with this PR #1343
@kof I investigated the work that @pranshuchittora did in this PR and to try to provide verification tests. To summarize:

1. @pranshuchittora found that this bug was introduced with v10.0.1 by the change to packages/react-jss/src/createUseStyles.js in this commit 9f7924e
2. His fix was to revert the change
3. @pranshuchittora produced a test that verified rendering multiple elements using media query and theme function (this was accepted as generally useful dynamicStyles test.) However, the test needs to additionally show that a viewport change would still render the multiple elements correctly.
4. @EZEDSEA confirmed that this reverted code does fix the issues in their code base. @mattbeaudry can comment as well on whether it fixes his code base.

An Effective Verification Test
For @pranshuchittora test to verify the fix, it needs to be extended/modified into two parts each demonstrating that the end result of applying the CSS occurred (different background changes based on viewport/window sizes):

1. Render component within a given window size (e.g., < 1024) so that the 2 wrapped MyComponent has background yellow
2. Change the window size (e.g., > 1024) so that the 2 wrapped MyComponent activate the @media query to set the background red

Limitations of react-test-renderer framework
The current react-test-renderer framework does not support viewport/window changes. Tests involving window size changes have been shown to work using Jest, Enzyme, and JSDom. They simulate window resize using JSDom by manually setting window.innerWidth and window.innerHeight.

After a window resize, JSDom needs to activate the @media query (setting some props and styles). The current version of JSDom still lists @media under "TODO". Digging deeper, this comment illustrates why it is not possible to use JSDom, react-test-renderer or even Jest, Enzyme for the 2-step test approach outlined above.

Where does this Leave this Open Issue #1320 and PR #1343
Here are some options to help unblock this PR for several projects that rely on this fix:

1. Need a different test approach than outlined above. Suggestions are welcomed. Perhaps any guidance about doing something similar to the hypothetical approach for JSS OR
2. Consider testing media queries through Cypress or similar framework where stylesheet actually get rendered OR
3. Take this PR fix(react-jss): Media query styles applied only to the first element in function #1343 fix which was known to work prior to the changes introduced by this commit 9f7924eb and confirmed to work by @EZEDSEA recently. OR
4. Taking Option 3 will mean that react-hot-loader feature supported by refactor of createUseStyles will need to be re-investigated.

Meta Issue
The need for tests is important particularly to verify this PR #1343 or the flaw with react-hot-loader implementation 9f7924eb. If there is a testing strategy that works with react-test-renderer it would be good to articulate. If it is not possible, what alternate testing framework could be used to verify? Would the repo owners approve such an endeavor?

[kof]

Thank you for the summary, I decided to look into the problem now. Here is a simplified reproduction I wish I had before

https://codesandbox.io/s/react-jss-playground-forked-sty9t?file=/index.js

[kof]

Given this code:

const useStyles = createUseStyles({
  wrapper: () => ({
    padding: 40,
    backgroundColor: "green",
    textAlign: "left",
    "@media (min-width: 1024px)": {
      backgroundColor: "red"
    }
  })
});

const Comp = () => {
  const classes = useStyles();
  return (
    <div className={classes.wrapper}>
      <h1>Hello React-JSS!</h1>
      <a href="http://cssinjs.org/react-jss" traget="_blank">
        See docs
      </a>
    </div>
  );
};

const App = () => (
  <React.Fragment>
    <Comp />
    <Comp />
  </React.Fragment>
);

render(<App />, document.getElementById("root"));

Registry contains correct CSS:

        .wrapper-0 {}
        .wrapper-d0-1 {
          padding: 40px;
          background: red;
          text-align: left;
        }
        @media (min-width: 1024px) {
          .wrapper-d0-1 {
            background-color: green;
          }
        }
          .wrapper-d1-2 {
            padding: 40px;
            background: red;
            text-align: left;
          }
        @media (min-width: 1024px) {
          .wrapper-d1-2 {
            background-color: green;
          }
        }

But the problem is that the second media query from a dynamic rule doesn't have the declaration inside when rendering in the browser:

[EZEDSEA]

@kof What can we do to help? You had mentioned unit tests didn't seem to test for this before. The fix that is proposed initially seems to just suggest a rollback of code from v10.0.1.

[kof]

@EZEDSEA I am searching for a solution that fixes es without reverting to the previous state. The PR that broke it in the first place was trying to fix hot reload, which is also important for many to work and has no tests whatsover.

It's super annoying for a maintainer to git 2 PRs one introduces changes that break one thing fixes another, next one reverts it and breaks the first one.

That is why without a test that is failing I am not going to merge anything at all, otherwise we will be stuck in this loop.

[kof]

If you want to help, for now don't worry about the test, but try find a minimal change that fixes the problem without breaking hot reload.

[kof]

So here is the real problem:

The problem is not in react-jss at all. It's the combination of jss-plugin-nested and jss-plugin-rule-value-function and overall plugins based architecture together with this particular interface.

  wrapper: () => ({
    padding: 40,
    backgroundColor: "green",
    textAlign: "left",
    "@media (min-width: 1024px)": {
      backgroundColor: "red"
    }
  })

This is just hard to make work efficiently. That function returns style object that contains a nested media query. To make it work on each update jss-plugin-rule-value-function has to add/update first level rule that includes a nested media query, then jss-plugin-nested needs to add/update that media rule along with it's child rules.

It essentially needs to transform the code above to this

      const styles = {
        wrapper: {
          padding: 40,
          background: 'red',
          textAlign: 'left'
        },
        '@media (min-width: 1024px)': {
          wrapper: {
            backgroundColor: 'green'
          }
        }
      }

There is a number of challenges with this:

1. It needs to avoid creating another media query if one already exists.
2. It needs to avoid creating a new child "wrapper" rule, but it needs to override it's styles in case they have changed
3. It needs to remove media queries that have been potentially created on previous update
4. It needs to remove previous child rules inside media query which might have been created potentially and now don't exist

This is a lot of diffing and logic to support, the easiest way forward is to not support nested media queries inside of function rules at all, maybe even warn about it.

A similar thing that works right now is and it has none of these problems.

      const styles = {
        wrapper: () => ({
          padding: 40,
          background: 'red',
          textAlign: 'left'
        }),
        '@media (min-width: 1024px)': {
          wrapper: () => ({
            backgroundColor: 'green'
          })
        }
      }

One property that is missing is that you can't generate the media condition based on props and for this we would need to find a separate solution. I could imagine a better interface for props based condition would be something like this:

      const styles = {
        wrapper: () => ({
          padding: 40,
          background: 'red',
          textAlign: 'left'
        }),
        '@media': {
          condition: (props) =>  '(min-width: 1024px)'
          wrapper: (props) => ({
            backgroundColor: 'green'
          })
        }
      }

[pybuche]

@kof 👋 Any update on this? I decided to switch from SASS to JSS on my project, but I now realize I can't create a "responsive system" to adapt the sizes of my components based on media queries.

Basically I've started to craft something like this:

mport _ from "lodash";
import { CSSProperties } from "react";

export const breakpoints = {
  xxs: 0,
  xs: 376,
  sm: 576,
  md: 768,
  lg: 992,
  xl: 1200,
  xxl: 1440,
} as const;

export type Breakpoint = keyof typeof breakpoints;

export type Responsive<T> = { [key in Breakpoint]?: T };

/**
 * Generates the media query key used in `createUseStyles`, to apply style on screens larger than `breakpoint`
 * See `responsiveHideUp` for an example
 * @param breakpoint
 */
export const responsiveUp = (breakpoint: Breakpoint) => {
  return `@media(min-width: ${breakpoints[breakpoint]}px)`;
};

/**
 * Generates the media query key used in `createUseStyles`, to apply style on screens smaller than `breakpoint`
 * See `responsiveHideDown` for an example
 * @param breakpoint
 */
export const responsiveDown = (breakpoint: Breakpoint) => {
  return `@media(max-width: ${breakpoints[breakpoint]}px)`;
};

/**
 * Style prop usable under `createUseStyles` to hide an elements on screens bigger than `breakpoint`
 * See murray's Frame implementation of `hideUp` for an example
 * @param breakpoint
 */
export const responsiveHideUp = (breakpoint: Breakpoint) => ({
  [responsiveUp(breakpoint)]: { display: "none" },
});

/**
 * Style prop usable under `createUseStyles` to hide an elements on screens small than `breakpoint`
 * See murray's Frame implementation of `hideDown` for an example
 * @param breakpoint
 */
export const responsiveHideDown = (breakpoint: Breakpoint) => ({
  [responsiveDown(breakpoint)]: { display: "none" },
});

to implement it in a design-systemed component like this:

type Props = {
  hideUp?: Breakpoint;
  hideDown?: Breakpoint;
  // ... other props
}

const useStyles = createUseStyles<string, Props>({
  frameStyle: ({
    hideUp,
    hideDown,
    className,
  }) => {
    const otherStyleProps = {}; // color, background etc

    const hideUpProps = hideUp ? responsiveHideUp(hideUp) : {};
    const hideDownProps = hideDown ? responsiveHideDown(hideDown) : {};

    return {
      composes: [className],
      // ... other props
      ...hideUpProps,
      ...hideDownProps,
    };
  },
});

and then use it like this:

<MyComponent hideUp="md" />
<MyComponent hideDown="xl" />

Do you think that's achievable? If we need to make a change to JSS, I can help, but I might need some pointers first! Thanks!

[kof]

seems like we got a successor for the same bug #1523