-
-
Notifications
You must be signed in to change notification settings - Fork 399
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
Fixes sheet ordering #819
Conversation
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@@ -268,7 +289,7 @@ function insertStyle(style: HTMLElement, options: PriorityOptions) { | |||
return | |||
} | |||
|
|||
getHead().insertBefore(style, prevNode) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I haven't found a better way. The logic there is still too complex, we might want to simplify it at some point, but I am fine with merging it now.
We should have supported an insertion point as node as the only argument, I think this would simplify it. |
I don't think it would simplify much because the only code which could be removed is: https://github.com/cssinjs/jss/blob/master/packages/jss/src/renderers/DomRenderer.js#L235-L243 |
* Added failing test for the ordering issue * Added fix for the issue * Update size-snapshots * Fix tests
Fixes the sheet ordering as described in #816
prevNode
returns now an object becausenextSibling
may return null when it's the last element which results in not being able to get the parent for the insertion.