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.

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

Jump to bottom

Fixes sheet ordering #819

Merged
merged 4 commits into from
Aug 12, 2018
Merged

Fixes sheet ordering #819

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c283ce7
Added failing test for the ordering issue
Aug 9, 2018
a966c51
Added fix for the issue
Aug 9, 2018
5c274f2
Update size-snapshots
Aug 11, 2018
a7a3990
Fix tests
Aug 12, 2018
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
41 changes: 31 additions & 10 deletions packages/jss/src/renderers/DomRenderer.js
View file
Open in desktop
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) {
if (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
View file
Open in desktop
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