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

Allow for pre-pending style tags to container element #1473

Merged
merged 8 commits into from Nov 25, 2019

Conversation

jcharry
Copy link
Contributor

@jcharry jcharry commented Aug 15, 2019

What: Currently, by default generated style tags are appended to the end of document.head. This can create issues with codebases that rely on passing className into components to override local component styles. Because the specificity of the passed in class often matches the class generated by emotion, the one generated by emotion pretty much always takes precedence because it is appended to the end of <head>.

By allowing the option for prepending tags to a container element, we can ensure that styles coming from css modules that are injected farther down in <head> will take precedence over emotions generated styles when there are specificity issues and the same css properties are being targeted.

How: the addition of a prepend property to createCache passes this property down to the StyleSheet where the inject method can read the property, and decide where to inject the style tag. If prepend is omitted, the current default behavior is preserved. This also attempts to preserve previous behavior such that if a before tag is found in the StyleSheet, the incoming new tag will be placed adjacent to it. Basically, this last bit is to preserve all default behavior except for the instance of someone passing prepend and no other style tags are available to be used as position markers.

Checklist:

  • Documentation
  • Tests N/A
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2019

🦋 Changeset is good to go

Latest commit: faaeca1

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #1473 into next will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/cache/src/index.js 98.9% <ø> (ø) ⬆️
packages/sheet/src/index.js 100% <100%> (ø) ⬆️

@jcharry
Copy link
Contributor Author

jcharry commented Aug 15, 2019

I'm not very familiar with the emotion codebase so this may not be what's actually needed to get the desired results. I also see there may be a different way of inserting rules in production, which this fix woulnd't address. Would very much appreciate some feedback/guidance on how to approach this.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

I don't think this is really something we should add, you can already achieve the same behavior by creating a div, inserting it in the head at the right position and making that div the container element.

@jcharry
Copy link
Contributor Author

jcharry commented Aug 22, 2019

I suppose that could work - but it's pretty non-semantic and not necessarily guaranteed to be supported by all browsers. Off the cuff I'm guessing divs in <head> will work, but it feels more natural to me to directly inject style tags.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 24, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit faaeca1:

Sandbox Source
Emotion Configuration

@Andarist Andarist added this to the v11 milestone Nov 24, 2019
@Andarist Andarist changed the base branch from master to next November 24, 2019 22:14
@Andarist
Copy link
Member

@mitchellhamilton this seems quite reasonable to me, I would be hesitant to put divs into head as well. I've simplified the implementation a little bit, so this is really a minor change and allows to achieve the goal in a cleaner way than multi-step workaround with a custom container. WDYT? Would you be willing to merge this?

@Andarist Andarist merged commit 4a891bf into emotion-js:next Nov 25, 2019
@Andarist
Copy link
Member

@jcharry thank you for your work! Sorry that you had to wait so long - this will become available in the upcoming v11 release.

@jcharry
Copy link
Contributor Author

jcharry commented Nov 25, 2019

@Andarist thanks so much!

louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
* Add prepend option to createCache

* changeset

* Simplify implementation a little bit

* Add test

* Tweak changeset

* Add new prepend option to TS types

* Add type for prepend in createCache options as well
@github-actions github-actions bot mentioned this pull request Nov 10, 2020
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.

None yet

3 participants