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

Add support for 'insertAt' parameter #69

Merged
merged 5 commits into from
Oct 15, 2015
Merged

Conversation

elsbree
Copy link
Contributor

@elsbree elsbree commented Jul 7, 2015

Implemented for #56. Adds an 'insertAt' parameter that lets you specify where you would like to insert the style tags in the head, either 'top' or 'bottom' (the default). This lets you avoid taking priority over any CSS already in the head if you so desire.

@sokra
Copy link
Member

sokra commented Jul 7, 2015

please use tabs instead of spaces... :)

@elsbree
Copy link
Contributor Author

elsbree commented Jul 8, 2015

Woops, somehow I didn't notice that. I changed the spaces to tabs.

head.insertBefore(styleElement, head.firstChild);
} else if (options.insertAt === "bottom") {
head.appendChild(styleElement);
}
Copy link
Member

Choose a reason for hiding this comment

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

else throw an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think of this for the error text: "Invalid value for parameter 'insertAt'. Must be 'top' or 'bottom'."

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

@vvo
Copy link

vvo commented Sep 15, 2015

https://github.com/substack/insert-css has "prepend" mode which I do find a good naming. What do you think @elsbree?

@elsbree
Copy link
Contributor Author

elsbree commented Sep 15, 2015

Seems like that would mostly be a question of semantics. The 'insertAt' parameter has the advantage of being more flexible, that is, we could allow options other than just 'top' or 'bottom' in the future. That being said, I don't have a strong preference either way.

@MattiSG
Copy link

MattiSG commented Oct 13, 2015

+1

@@ -45,6 +45,10 @@ Note: Behavior is undefined when `unuse`/`unref` is called more often than `use`

### Options

#### `insertAt`

By default, the style-loader appends `<style>` elements to the end of the `<head>` tag of the page. This will cause CSS created by the loader to take priority over CSS already present in the document head. To insert style elements at the beginning of the head, set this query parameter to 'bottom', e.g. `require('../style.css?insertAt=bottom')`.
Copy link
Member

Choose a reason for hiding this comment

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

- 'bottom', e.g. `require('../style.css?insertAt=bottom')`
+ 'top', e.g. `require('!style?insertAt=bottom!css!../style.css')`

sokra added a commit that referenced this pull request Oct 15, 2015
Add support for 'insertAt' parameter
@sokra sokra merged commit 3944f22 into webpack-contrib:master Oct 15, 2015
@sokra
Copy link
Member

sokra commented Oct 15, 2015

Thanks

@sokra
Copy link
Member

sokra commented Oct 15, 2015

Hmm... It's broken, because style tags are in incorrect order.

i. e.

require("a.css");
require("b.css");

bottom:

<head>
  <style src="existing"></style>
  <style>... a.css ...</style>
  <style>... b.css ...</style>
</head>

top:

<head>
  <style>... b.css ...</style>
  <style>... a.css ...</style>
  <style src="existing"></style>
</head>

and the order is important...

Could you do a follow-up PR that fix this, i. e. by storing a list of all top-inserted style tags and inserting after the last one. Keep in mind that top and bottom could be mixed?

@elsbree
Copy link
Contributor Author

elsbree commented Oct 15, 2015

Oh, good catch. Yep, I can work on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants