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 media query issue #1523

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mjeanroy
Copy link

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

  • Add test(s) that verify the modified behavior
  • Add documentation if it changes public API

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 🙏

@mjeanroy mjeanroy requested a review from kof as a code owner June 23, 2021 15:22
// $FlowFixMe[incompatible-use]
// $FlowFixMe[prop-missing]
.addRule(styleRule.key, style[prop], {selector: styleRule.selector})
const queue = container.prepareQueue()
Copy link
Author

Choose a reason for hiding this comment

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

I think that this code exposes too much internal API, it probably needs to be changed.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, definitely, need to find a cleaner way

Copy link
Member

Choose a reason for hiding this comment

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

The logic with the queue is a mess :)

Copy link
Member

@kof kof Jun 27, 2021

Choose a reason for hiding this comment

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

It wasn't clean before, but now with queue from the instance and from an argument in deployRule, it became even worse

@kof
Copy link
Member

kof commented Jun 27, 2021

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

Choose a reason for hiding this comment

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

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.

@kof
Copy link
Member

kof commented Jun 27, 2021

I think you are on a good path, we need tests and we need a much cleaner api.

@kof kof added the needs tests We can't merge a change without tests verifying it's behaviour label Jun 27, 2021
@hkar19
Copy link

hkar19 commented Jul 10, 2021

may we have patch package for this commit please?

@caitlinwhiteley
Copy link

Hi, are there any updates on the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests We can't merge a change without tests verifying it's behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants