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

Handle placeholder strings #2

Merged
merged 2 commits into from
Oct 18, 2017
Merged

Handle placeholder strings #2

merged 2 commits into from
Oct 18, 2017

Conversation

andrewb
Copy link
Contributor

@andrewb andrewb commented Oct 17, 2017

This PR wraps placeholder strings in CSS comments before processing. After the CSS has been processed the placeholders are unwrapped. This is similar to the approach in styled-jsx-plugin-sass.

This allows the following to be processed:

    <style jsx>{`
      div {
        color: red;
        ${style}
      }
    `}</style>

Without this change the above code throws CssSyntaxError: <css input>:3:3: Unknown word.

index.js Outdated
@@ -2,6 +2,10 @@ const loopWhile = require('deasync').loopWhile
const processor = require('./processor')

module.exports = (css, settings) => {
const cssWithPlaceholders = css
.replace(/%%styled-jsx-(expression|placeholder)-(\d+)%%/g, (_, p, id) =>
Copy link
Owner

@giuseppeg giuseppeg Oct 18, 2017

Choose a reason for hiding this comment

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

I made a mistake and never meant to use expression here. It should be %%styled-jsx-placeholder-(\d+)%%
Can you revert to the original version and just replace expression- with placeholder-?

@andrewb
Copy link
Contributor Author

andrewb commented Oct 18, 2017

Thanks @giuseppeg. Updated as requested.

@giuseppeg giuseppeg merged commit 0a5dcb6 into giuseppeg:master Oct 18, 2017
@ryanfitzer
Copy link

I think this needs to be revisited. Currently, PostCSS removes all comments from the processed css:

PostCSS cleans values from comments by design.

So, since that comment is over a year before this PR, I'm a bit confused about the goal for this PR. I can understand why it fixed the CssSyntaxError issue, but it makes it so template literals are no longer able to be used ("dynamic css") as the value of a css property.

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