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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/css-jss/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"css-jss.js": {
"bundled": 61186,
"minified": 21939,
"gzipped": 7380
"bundled": 61681,
"minified": 22104,
"gzipped": 7434
},
"css-jss.min.js": {
"bundled": 60111,
"minified": 21316,
"gzipped": 7100
"bundled": 60606,
"minified": 21481,
"gzipped": 7155
},
"css-jss.cjs.js": {
"bundled": 3034,
Expand Down
24 changes: 12 additions & 12 deletions packages/jss-plugin-global/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"jss-plugin-global.js": {
"bundled": 5195,
"minified": 2291,
"gzipped": 941
"bundled": 5562,
"minified": 2405,
"gzipped": 969
},
"jss-plugin-global.min.js": {
"bundled": 5195,
"minified": 2291,
"gzipped": 941
"bundled": 5562,
"minified": 2405,
"gzipped": 969
},
"jss-plugin-global.cjs.js": {
"bundled": 4452,
"minified": 2417,
"gzipped": 913
"bundled": 4803,
"minified": 2531,
"gzipped": 947
},
"jss-plugin-global.esm.js": {
"bundled": 4106,
"minified": 2143,
"gzipped": 808,
"bundled": 4457,
"minified": 2257,
"gzipped": 841,
"treeshaked": {
"rollup": {
"code": 55,
Expand Down
14 changes: 14 additions & 0 deletions packages/jss-plugin-global/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,25 @@ class GlobalContainerRule implements ContainerRule {
* Create and register rule, run plugins.
*/
addRule(name, style, options) {
return this.createRule(name, style, options)
}

// eslint-disable-next-line class-methods-use-this
prepareQueue() {
// NoOp
}

createRule(name, style, options) {
const rule = this.rules.add(name, style, options)
if (rule) this.options.jss.plugins.onProcessRule(rule)
return rule
}

// eslint-disable-next-line class-methods-use-this
deployRule() {
// NoOp
}

/**
* Get index of a rule.
*/
Expand Down
24 changes: 12 additions & 12 deletions packages/jss-plugin-nested/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
{
"jss-plugin-nested.js": {
"bundled": 4795,
"minified": 1622,
"gzipped": 885
"bundled": 4923,
"minified": 1673,
"gzipped": 918
},
"jss-plugin-nested.min.js": {
"bundled": 4355,
"minified": 1406,
"gzipped": 757
"bundled": 4483,
"minified": 1457,
"gzipped": 791
},
"jss-plugin-nested.cjs.js": {
"bundled": 4052,
"minified": 1582,
"gzipped": 811
"bundled": 4176,
"minified": 1633,
"gzipped": 845
},
"jss-plugin-nested.esm.js": {
"bundled": 3633,
"minified": 1259,
"gzipped": 696,
"bundled": 3757,
"minified": 1310,
"gzipped": 732,
"treeshaked": {
"rollup": {
"code": 64,
Expand Down
17 changes: 10 additions & 7 deletions packages/jss-plugin-nested/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,16 @@ export default function jssNested(): Plugin {
container.addRule(selector, style[prop], {...options, selector})
} 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})
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

const addedRule = container.createRule(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]
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.


container.deployRule(addedRule, queue)
}

delete style[prop]
Expand Down
12 changes: 6 additions & 6 deletions packages/jss-preset-default/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"jss-preset-default.js": {
"bundled": 58386,
"minified": 21163,
"gzipped": 7032
"bundled": 58881,
"minified": 21328,
"gzipped": 7081
},
"jss-preset-default.min.js": {
"bundled": 57311,
"minified": 20540,
"gzipped": 6753
"bundled": 57806,
"minified": 20705,
"gzipped": 6803
},
"jss-preset-default.cjs.js": {
"bundled": 2222,
Expand Down
12 changes: 6 additions & 6 deletions packages/jss-starter-kit/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"jss-starter-kit.js": {
"bundled": 74529,
"minified": 31614,
"gzipped": 9665
"bundled": 75024,
"minified": 31779,
"gzipped": 9720
},
"jss-starter-kit.min.js": {
"bundled": 73454,
"minified": 31426,
"gzipped": 9442
"bundled": 73949,
"minified": 31591,
"gzipped": 9496
},
"jss-starter-kit.cjs.js": {
"bundled": 5647,
Expand Down
28 changes: 14 additions & 14 deletions packages/jss/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
{
"jss.js": {
"bundled": 63839,
"minified": 23351,
"gzipped": 7097
"bundled": 64256,
"minified": 23567,
"gzipped": 7140
},
"jss.min.js": {
"bundled": 62442,
"minified": 22583,
"gzipped": 6747
"bundled": 62859,
"minified": 22799,
"gzipped": 6789
},
"jss.cjs.js": {
"bundled": 59531,
"minified": 26202,
"gzipped": 7199
"bundled": 59922,
"minified": 26418,
"gzipped": 7246
},
"jss.esm.js": {
"bundled": 57923,
"minified": 24913,
"gzipped": 7026,
"bundled": 58314,
"minified": 25129,
"gzipped": 7069,
"treeshaked": {
"rollup": {
"code": 20340,
"code": 20556,
"import_statements": 345
},
"webpack": {
"code": 21825
"code": 22041
}
}
}
Expand Down
34 changes: 25 additions & 9 deletions packages/jss/src/StyleSheet.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,21 +83,39 @@ export default class StyleSheet {
* Will insert a rule also after the stylesheet has been rendered first time.
*/
addRule(name: string, decl: JssStyle, options?: RuleOptions): Rule | null {
const {queue} = this
const queue = this.prepareQueue()
const rule = this.createRule(name, decl, options)

// Plugins can create rules.
// In order to preserve the right order, we need to queue all `.addRule` calls,
// which happen after the first `rules.add()` call.
if (this.attached && !queue) this.queue = []
if (!rule) return null

this.deployRule(rule, queue)

return rule
}

createRule(name: string, decl: JssStyle, options?: RuleOptions): Rule | null {
const rule = this.rules.add(name, decl, options)

if (!rule) return null

this.prepareQueue()
this.options.jss.plugins.onProcessRule(rule)
return rule
}

prepareQueue(): ?Array<Rule> {
const {queue} = this

// Plugins can create rules.
// In order to preserve the right order, we need to queue all `.addRule` calls,
// which happen after the first `rules.add()` call.
if (this.attached && !queue) this.queue = []
return queue
}

deployRule(rule: Rule, queue: ?Array<Rule>): void {
if (this.attached) {
if (!this.deployed) return rule
if (!this.deployed) return
// Don't insert rule directly if there is no stringified version yet.
// It will be inserted all together when .attach is called.
if (queue) queue.push(rule)
Expand All @@ -108,14 +126,12 @@ export default class StyleSheet {
this.queue = undefined
}
}
return rule
return
}

// We can't add rules to a detached style node.
// We will redeploy the sheet once user will attach it.
this.deployed = false

return rule
}

/**
Expand Down
12 changes: 6 additions & 6 deletions packages/react-jss/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
{
"react-jss.js": {
"bundled": 169770,
"minified": 59580,
"gzipped": 19568
"bundled": 170265,
"minified": 59745,
"gzipped": 19621
},
"react-jss.min.js": {
"bundled": 112410,
"minified": 42780,
"gzipped": 14522
"bundled": 112905,
"minified": 42945,
"gzipped": 14573
},
"react-jss.cjs.js": {
"bundled": 27701,
Expand Down