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

Fixes sheet ordering #819

Merged
merged 4 commits into from
Aug 12, 2018
Merged
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
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 @@
{
"dist/jss.js": {
"bundled": 70577,
"minified": 20173,
"gzipped": 6124
"bundled": 70962,
"minified": 20276,
"gzipped": 6145
},
"dist/jss.min.js": {
"bundled": 69339,
"minified": 19555,
"gzipped": 5805
"bundled": 69724,
"minified": 19658,
"gzipped": 5827
},
"dist/jss.cjs.js": {
"bundled": 48406,
"minified": 20842,
"gzipped": 5950
"bundled": 48623,
"minified": 20945,
"gzipped": 5971
},
"dist/jss.esm.js": {
"bundled": 47981,
"minified": 20488,
"gzipped": 5866,
"bundled": 48198,
"minified": 20591,
"gzipped": 5887,
"treeshaked": {
"rollup": {
"code": 17463,
"code": 17566,
"import_statements": 85
},
"webpack": {
"code": 18755
"code": 18858
}
}
}
Expand Down
41 changes: 31 additions & 10 deletions packages/jss/src/renderers/DomRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,45 +216,66 @@ function findCommentNode(text: string): Node | null {
return null
}

type PrevNode = {
parent: ?Node,
node: ?Node
}

/**
* Find a node before which we can insert the sheet.
*/
function findPrevNode(options: PriorityOptions): ?Node | null {
function findPrevNode(options: PriorityOptions): PrevNode | false {
const {registry} = sheets

if (registry.length > 0) {
// Try to insert before the next higher sheet.
let sheet = findHigherSheet(registry, options)
if (sheet) return sheet.renderer.element
if (sheet) {
return {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like calling parentNode in advance doesn't bring much value other than makes findPrevNode semantics wrong. Mb I oversee something but to me it looks like we could keep the return value as it was.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is when returning nextSibling and when it returns null we can't get the parentNode

Copy link
Member

Choose a reason for hiding this comment

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

Arent you appending in the end of the head when nextSibling is null?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily, it was supposed to work with appending it at the end of the parent of the stylesheet

parent: sheet.renderer.element.parentNode,
node: sheet.renderer.element
}
}

// Otherwise insert after the last attached.
sheet = findHighestSheet(registry, options)
if (sheet) return sheet.renderer.element.nextElementSibling
if (sheet) {
return {
parent: sheet.renderer.element.parentNode,
node: sheet.renderer.element.nextSibling
}
}
}

// Try to find a comment placeholder if registry is empty.
const {insertionPoint} = options
if (insertionPoint && typeof insertionPoint === 'string') {
const comment = findCommentNode(insertionPoint)
if (comment) return comment.nextSibling
if (comment) {
return {
parent: comment.parentNode,
node: comment.nextSibling
}
}

// If user specifies an insertion point and it can't be found in the document -
// bad specificity issues may appear.
warning(insertionPoint === 'jss', '[JSS] Insertion point "%s" not found.', insertionPoint)
}

return null
return false
}

/**
* Insert style element into the DOM.
*/
function insertStyle(style: HTMLElement, options: PriorityOptions) {
const {insertionPoint} = options
const prevNode = findPrevNode(options)
const nextNode = findPrevNode(options)

if (nextNode !== false && nextNode.parent) {
nextNode.parent.insertBefore(style, nextNode.node)

if (prevNode) {
const {parentNode} = prevNode
if (parentNode) parentNode.insertBefore(style, prevNode)
return
}

Expand All @@ -268,7 +289,7 @@ function insertStyle(style: HTMLElement, options: PriorityOptions) {
return
}

getHead().insertBefore(style, prevNode)
Copy link
Member

Choose a reason for hiding this comment

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

it worked basically before same way, because the insertBefore api allows to pass prevNode === null and it will be inserted in the end of the parent, so the same as appendChild

Copy link
Member Author

Choose a reason for hiding this comment

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

seems a bit simpler to me

getHead().appendChild(style)
}

/**
Expand Down
8 changes: 6 additions & 2 deletions packages/jss/tests/functional/priority.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,29 +189,33 @@ describe('Functional: dom priority', () => {
})
})

describe('element insertion point', () => {
describe('element insertion point: body', () => {
let insertionPoint
let sheet1
let sheet2
let sheet3
beforeEach(() => {
insertionPoint = document.body.appendChild(document.createElement('div'))
const jss = create({insertionPoint})
sheet2 = jss.createStyleSheet({}, {meta: 'sheet2', index: 2}).attach()
sheet1 = jss.createStyleSheet({}, {meta: 'sheet1', index: 1}).attach()
sheet3 = jss.createStyleSheet({}, {meta: 'sheet3', index: 3}).attach()
})

afterEach(() => {
document.body.removeChild(insertionPoint)
sheet2.detach()
sheet1.detach()
sheet3.detach()
})

it('should insert sheets in the correct order', () => {
const styleElements = document.body.getElementsByTagName('style')

expect(styleElements.length).to.be(2)
expect(styleElements.length).to.be(3)
expect(styleElements[0].getAttribute('data-meta')).to.be('sheet1')
expect(styleElements[1].getAttribute('data-meta')).to.be('sheet2')
expect(styleElements[2].getAttribute('data-meta')).to.be('sheet3')
})
})

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 @@
{
"dist/react-jss.js": {
"bundled": 236824,
"minified": 65331,
"gzipped": 17972
"bundled": 237212,
"minified": 65434,
"gzipped": 17994
},
"dist/react-jss.min.js": {
"bundled": 201522,
"minified": 55978,
"gzipped": 15787
"bundled": 201910,
"minified": 56081,
"gzipped": 15810
},
"dist/react-jss.cjs.js": {
"bundled": 17646,
Expand Down