Fix media query issue #1523
Fix media query issue #1523
Conversation
| // $FlowFixMe[incompatible-use] | ||
| // $FlowFixMe[prop-missing] | ||
| .addRule(styleRule.key, style[prop], {selector: styleRule.selector}) | ||
| const queue = container.prepareQueue() |
There was a problem hiding this comment.
I think that this code exposes too much internal API, it probably needs to be changed.
There was a problem hiding this comment.
yeah, definitely, need to find a cleaner way
There was a problem hiding this comment.
The logic with the queue is a mess :)
There was a problem hiding this comment.
It wasn't clean before, but now with queue from the instance and from an argument in deployRule, it became even worse
|
I am a bit surprised that this could be a fix for that linked bug. Can you please create a failing test? A test would be super valuable here to make sure we are fixing the same right thing. Ideally right now we need a test that shows we are fixing the originally reported bug: in react-jss that would use a media query inside a function rule, like in the original description. Later on we would need a test in plugin nested and in the core if we end up modifying/adding those methods, but for now we need a test in react-jss because that would make sure the entire integration works as expected. |
| // And flow doesn't know this will always be a StyleRule which has the addRule method | ||
| // $FlowFixMe[incompatible-use] | ||
| // $FlowFixMe[prop-missing] | ||
| addedRule.addRule(styleRule.key, style[prop], {selector: styleRule.selector}) |
There was a problem hiding this comment.
Essentially all this is about ability to call an .addRule on a rule that implements ContainerRule without rendering anything (only adding to a queue) and then deploying the container rule as a next step. Is that correct?
I think it would be much easier to find a better API that allows this if we had tests to verify the behavior, because right now trying things out without a test is way too hard.
|
I think you are on a good path, we need tests and we need a much cleaner api. |
kof
added
the
needs tests
|
may we have patch package for this commit please? |
|
Hi, are there any updates on the status of this PR? |
Corresponding Issue(s):
#1320
What Would You Like to Add/Fix?
The goal is to fix an issue regarding media query rules when defined in a nested & dynamic rule.
Todo
Expectations on Changes
/
Changelog
I already gave a detailed explanation here.
Please note that this PR is just a basic fix, it's probably not clean but I open this PR to get your feedback on this 馃檹