-
-
Notifications
You must be signed in to change notification settings - Fork 398
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
Media query styles applied only to the first element in function #1320
Comments
@kof if this issue is still open then I would like to work on this :) |
@pranshuchittora sure |
Thanks @kof for the blazing fast reply 👍 |
- wrapper: () => ({
padding: 40,
background: theme.background,
textAlign: "left",
"@media (min-width: 1024px)": {
backgroundColor: "red"
}
- }),
+ wrapper: {
padding: 40,
background: theme.background,
textAlign: "left",
"@media (min-width: 1024px)": {
backgroundColor: "red"
},
+ }, @karpcs Converting the style object from an arrow function to plain object works. @kof probably this is happening due to improper object referencing in arrow function approach. Is it still a bug, if no then I suggest closing this issue :) |
Feel free to dig deeper and figure out what the actual bug is. Yes it is only when rule function is used. |
Same as #1327 |
Might be just the playground, also it's not on the latest version, feel free to update and check what it does. |
I have updated the deps. "jss": "^10.1.1",
"jss-preset-default": "^10.1.1" Still not working, will take a look at the rule function parsing. |
Hi @kof , Did some preliminary research and found out that the function is not being executed to get the returned object as shown below. As the function is not bein executed, therefore converting it toCss string returns |
I kept the version in playground at the version, that this bug appears, prior versions does not have this bug. i noticed this when looping through array and passing different props to each and only the first element got the styles, so i just put a little demo here |
@pranshuchittora yes, you are still on the surface, sheet.update is supposed to call function rules, jss-plugin-rule-value-function plugin is responsible for implementing the function rule API ... somewhere between jss core and the plugin there is a bug, most likely in the plugin. |
https://github.com/cssinjs/repl/blob/gh-pages/src/index.js#L55 repl doesn't call .update, so on the repl function rules won't work, we need to start calling .update there. The actual issue in your production code might be in react-jss |
Yeah, after call |
Till v10.0.0 it was working perfectly. In The line -> jss/packages/jss/src/DomRenderer.js Line 406 in d9cca31
Is trying to concert the dynamic style to string, which is unable to. This flow is encountered when the P.S. |
That line was changed last time before 10.0.0 |
Hi @kof , Reverting the changes in |
Can you create a test? And optionally a fix?
…On Sat, May 9, 2020, 11:31 Pranshu Chittora ***@***.***> wrote:
Hi @kof <https://github.com/kof> ,
So I ran git bisect between the v10.0.0 & v10.0.1. And found that the
commit which introduced that bug was 9f7924e
<9f7924e>
.
[image: Screenshot from 2020-05-09 14-25-42]
<https://user-images.githubusercontent.com/32242596/81469474-a0960e80-9202-11ea-8a79-b4c0983f3d28.png>
Reverting the changes in packages/react-jss/src/createUseStyles.js
introduced in the following commit fixes the bug.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1320 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAM4WAFNYSXDJPUS5IZHLDRQUPGFANCNFSM4LTOR7UA>
.
|
This is still an issue, correct? I am on |
Hi @kof, I tried to debug this, and I would like to share with you what I found.
If we wrap The issue really occurs when updating a dynamic rule, in fact, if we do: if (newSheet) {
// Detach the sheet so all rules will be inserted
unmanageSheet({
index,
context,
sheet: newSheet,
theme
});
manageSheet({
index,
context,
sheet: newSheet,
theme
});
} => It also fixes the issue. I tried to go deeper to understand why updating dynamic rules did not works as exepected, and I think the issue is in } else if (isNestedConditional) {
// Place conditional right after the parent rule to ensure right ordering.
container
.addRule(prop, {}, options)
// Flow expects more options but they aren't required
// And flow doesn't know this will always be a StyleRule which has the addRule method
// $FlowFixMe[incompatible-use]
// $FlowFixMe[prop-missing]
.addRule(styleRule.key, style[prop], {selector: styleRule.selector})
} The line I tried to fix it by splitting the rule addition and the insertion, something like this: const addedRule = container.createRule(prop, {}, options);
addedRule.addRule(styleRule.key, style[prop], {selector: styleRule.selector})
container.deployRule(addedRule); You can find the commit on my fork here. Note that it is just a POC, I did not try to make it "super clean", but if you think the solution seems OK, feel free to give your opinion and I can work on a PR :) In the meantime, I would like to suggest a workaround until the issue is fixed: adding a plugin that detach and re-attach the sheet, such as: jss.use({
onUpdate(data: any, rule, sheet, options) {
if (sheet) {
sheet.detach().attach();
}
},
}); This workaround can definitely be improved, that is just the basic idea :) Also, thanks for your amazing library, that's an impressive work, and I really love using it [EDIT] I opened #1523 to get your feedback |
definitely @mjeanroy suggestion to use: jss.use({
onUpdate(data: any, rule, sheet, options) {
if (sheet) {
sheet.detach().attach();
}
},
}); is very nice. I just want to add, if you are using react-jss, remember that using therefore, you need to do things manually such as:
import { create as createJss, Rule, StyleSheet } from "jss";
import { JssProvider, ThemeProvider } from "react-jss";
import preset from "jss-preset-default";
// i made my array of plugins here, and spread it in my jss.use
import jssPlugins from "./jssPlugins";
// dont forget this!
const jss = createJss(preset());
jss.use(
{
onUpdate(data: object, rule: Rule, sheet: StyleSheet) {
if (sheet) {
sheet.detach().attach();
}
},
},
...jssPlugins
);
ReactDOM.render(
<React.StrictMode>
<JssProvider jss={jss}>
<ThemeProvider theme={theme}>
<App />
</ThemeProvider>
</JssProvider>
</React.StrictMode>,
document.getElementById("root")
);
import a from "jss-plugin-camel-case";
import b from "jss-plugin-compose";
import c from "jss-plugin-default-unit";
import d from "jss-plugin-expand";
import e from "jss-plugin-extend";
import f from "jss-plugin-global";
import g from "jss-plugin-nested";
import h from "jss-plugin-props-sort";
import i from "jss-plugin-rule-value-function";
import j from "jss-plugin-rule-value-observable";
import k from "jss-plugin-template";
import l from "jss-plugin-vendor-prefixer";
// add any more plugins you desire
export default [a(), b(), c(), d(), e(), f(), g(), h(), i(), j(), k(), l()]; this workaround is tedious, but for now this is all we got UPDATE: actually this method will give some annoyance in storybook where some styles got lost if you are using style as function. if you're using style as object, it would be working fine. for example: content: {
flex: 1,
overflowY: "scroll",
// overflowX: ""
maxHeight: "100vh",
backgroundColor: theme.defaultBackground,
} some of these style will be lost somehow, (only in storybook, btw). content: ()=>({
flex: 1,
overflowY: "scroll",
// overflowX: ""
maxHeight: "100vh",
backgroundColor: theme.defaultBackground,
}) |
Hello, any updates on a fix for this? Experiencing this issue on |
Sorry if I offended or don't understand your processes, I'm speaking as a regular consumer of your library This bug is from version 10.0.1. Shouldn't it be fixed by the current library team? Part of my team is giving up on jss, as well as some people I know because of this problem. I honestly don't quite understand the point of preparing a move to 18 React if there are such critical bugs |
I am merging good PRs that make progress, if a PR ads support for react 18 and has tests etc - cool. if you want to get something fixed, please fix it, add a test - I will merge it too. |
Expected behavior:
when changing screen size media query in function should apply styles to all the component that uses it
Describe the bug:
only the first component gets the styles of media query in function, it looks like it was introduced in 10.0.1
Codesandbox link:
https://codesandbox.io/s/react-jss-playground-nwurq
Versions (please complete the following information):
The text was updated successfully, but these errors were encountered: