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

Fix hashes should match when using a single tag #292

Merged
merged 1 commit into from
Oct 4, 2017
Merged

Fix hashes should match when using a single tag #292

merged 1 commit into from
Oct 4, 2017

Conversation

giuseppeg
Copy link
Collaborator

When there is only a single style tag per component we still hashString its hash (since we always do hashString(hashes) where hashes is an array). The resulting hash is used to scope CSS.

However the styleId for JSXStyle then used just hashes[0] resulting in something like this:

<_JSXStyle styleId="1234" css="div.jsx-9887 { color: red }" />

Instead with this fix it will match:

<_JSXStyle styleId="9887" css="div.jsx-9887 { color: red }" />

Again, this was an issue only when there was a single style tag per component. When there are multiple it is normal that styleId and the hash in the css don't match

@giuseppeg giuseppeg requested a review from nkzawa October 3, 2017 17:14
@giuseppeg
Copy link
Collaborator Author

cc @timneutkens

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Awesome lgtm! ❤️

@timneutkens
Copy link
Member

Could you do a new release after merging. Then we can add it to next v4 👌

@rauchg rauchg merged commit 2604fd3 into master Oct 4, 2017
@rauchg rauchg deleted the fix-hash branch October 4, 2017 13:32
rauchg added a commit that referenced this pull request Oct 4, 2017
@giuseppeg
Copy link
Collaborator Author

@rauchg I should have tested with next-news before merging. Did you find any bug? I just saw https://github.com/zeit/styled-jsx/tree/revert-292-fix-hash

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.

3 participants