Skip to content

Fix added nested rules order #655

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.

Sign up for GitHub

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

Closed
lttb wants to merge 2 commits into from
Closed

Fix added nested rules order #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2018cba
Fix nested styles order
lttb Dec 22, 2017
94ffff2
Add tests for added nested rules order
lttb Dec 22, 2017
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
2 changes: 1 addition & 1 deletion src/StyleSheet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ export default class StyleSheet {
if (!this.deployed) return rule
// 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)
if (queue) queue.unshift(rule)
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

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

I am not sure why this is not breaking anything else. You are reverting the order of added rules for all addRule calls. Maybe there is a missing test which ensures the right order.

We have actually an options.index which is set in jss-nested in order to have the control over the order. Maybe jss-nested sets the wrong index?

else {
this.insertRule(rule)
if (this.queue) {
Expand Down
32 changes: 31 additions & 1 deletion tests/integration/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import expect from 'expect.js'
import {stripIndent} from 'common-tags'
import jssNested from 'jss-nested'
import {create} from '../../src'
import StyleSheet from '../../src/StyleSheet'
import {createGenerateClassName} from '../utils'
import PluginsRegistry from '../../src/PluginsRegistry'
import {
createGenerateClassName,
getCssFromSheet,
removeWhitespace
} from '../utils'

describe('Integration: plugins', () => {
let jss
Expand Down Expand Up @@ -416,4 +421,29 @@ describe('Integration: plugins', () => {
`)
})
})

describe('jss-nested', () => {
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

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

this test actually belongs to jss-nested

Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

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

alternatively we could describe the same scenario using plugins api instead of jss-nested plugin

Copy link
Copy Markdown
Member Author

@lttb lttb Dec 22, 2017

Choose a reason for hiding this comment

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

I think that this test should be here because it tests the way how rules apply in some cases, so the same scenario instead of jss-nested looks better for me

let sheet

beforeEach(() => {
jss.use(jssNested())

sheet = jss.createStyleSheet({}, {
link: true,
}).attach()

sheet.addRule('b', {color: 'green'})
sheet.addRule('a', {
'&:hover': {
'& $b': {
color: 'red',
},
},
})
})

it('should save the added nested rules order', () => {
expect(getCssFromSheet(sheet)).to.be(removeWhitespace(sheet.toString()))
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

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

we only use DOM based tests in functional tests, is DOM really needed here or sheet.toString() + comparing with result would be enough?

Copy link
Copy Markdown
Member Author

@lttb lttb Dec 22, 2017

Choose a reason for hiding this comment

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

yeah, I think we can replace expect with the exact result

})
})
})
⚡