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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pranshuchittora
Copy link
Contributor

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

@pranshuchittora pranshuchittora changed the title Pc/fix/react jss/dynamic sheet fix(react-jss): Media query styles applied only to the first element in function May 9, 2020
@kof
Copy link
Member

kof commented May 11, 2020

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

@kof
Copy link
Member

kof commented Jun 3, 2020

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

@pranshuchittora
Copy link
Contributor Author

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

@anthonygrignoux
Copy link

Hello, is there any news about that PR?

@pranshuchittora
Copy link
Contributor Author

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

}

describe('React-JSS: createUseStyles', () => {
createBasicTests({createStyledComponent})
it('should render multiple elements with applied media query', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

kof commented Aug 4, 2020

hey @pranshuchittora, any updates?

@pranshuchittora
Copy link
Contributor Author

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

@pranshuchittora
Copy link
Contributor Author

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
Copy link
Member

kof commented Sep 23, 2020

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

@kof kof reopened this Sep 23, 2020
@pranshuchittora
Copy link
Contributor Author

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

@alee8
Copy link

alee8 commented Dec 1, 2020

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
Copy link

@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
Copy link

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

@jonathan-l-rivera
Copy link

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

@alee8
Copy link

alee8 commented Jan 7, 2021

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
Copy link
Member

kof commented Jan 7, 2021

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
Copy link

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
Copy link
Member

kof commented Jan 7, 2021

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
Copy link

EZEDSEA commented Jan 11, 2021

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
Copy link
Member

kof commented Jan 11, 2021

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

@kof
Copy link
Member

kof commented Jan 18, 2021

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

@kof
Copy link
Member

kof commented Jan 18, 2021

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

@EZEDSEA
Copy link

EZEDSEA commented Jan 19, 2021

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
Copy link
Member

kof commented Jan 19, 2021

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
Copy link

alee8 commented Jan 31, 2021

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
Copy link
Member

kof commented Mar 22, 2021

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
Copy link
Member

kof commented Mar 22, 2021

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:

Screenshot 2021-03-22 at 11 52 47

@EZEDSEA
Copy link

EZEDSEA commented Mar 23, 2021

@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
Copy link
Member

kof commented Mar 24, 2021

@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
Copy link
Member

kof commented Mar 24, 2021

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
Copy link
Member

kof commented Mar 27, 2021

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
Copy link

pybuche commented Jun 21, 2021

@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
Copy link
Member

kof commented Jun 27, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media query styles applied only to the first element in function
8 participants