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

v4.0.0 #31

Merged
merged 4 commits into from
Jul 15, 2017
Merged

v4.0.0 #31

merged 4 commits into from
Jul 15, 2017

Conversation

MicheleBertoli
Copy link
Member

Use placeholder class names.

@geelen
Copy link
Member

geelen commented Jun 19, 2017

Interesting! Why did you use indexes className="c0" instead of just replacing the whole thing with something like className="__SC__" (or even className="💅" 😜)?

@MicheleBertoli
Copy link
Member Author

MicheleBertoli commented Jun 19, 2017

Hey @geelen, thanks for your comment.
I decided to go with indexes because one of the best use case for snapshots is checking them during code reviews, therefore I needed a way to easily associate the styles with their elements.

exports[`toMatchSnapshot test-renderer 1`] = `
.c0 {
  padding: 4em;
  background: papayawhip;
}

.c1 {
  font-size: 1.5em;
  -webkit-text-align: center;
  text-align: center;
  color: palevioletred;
}

<section
  className="c0"
>
  <h1
    className="c1"
  >
    Hello World, this is my first styled component!
  </h1>
</section>
`;

I tried a generic placeholder first but it didn't work very well.
I hope this makes sense for you, but if you see any reason why I should avoid indexes I'll be more than happy to find a different solution.

Also, emojis would be awesome.
I always mention your website in my talks and posts, as one of the most important reasons to use CSSinJS :)

@rikukissa
Copy link
Contributor

I really like this PR! We just had some discussions about our project's snapshots being cumbersome and hard to maintain and this could definitely be an answer for that. The snapshots have kind of lost their meaning and value as they seem to be really fragile in their current form. Even the smallest style change cause nasty merge conflicts since many snapshots are changing.

Went through the code changes and it all looks good to me. Started actually using this already in our project and it seems to work just as expected. Amazing work 👍

@kentcdodds
Copy link
Member

@rikukissa, what happens when you add another styled component in the mix? Does the placeholder get bumped in a way that makes the diff hard to review?

@rikukissa
Copy link
Contributor

@kentcdodds excellent point. Oh yeah, you are right. Adding a new styled component surely messes up the diffs 😭

@MicheleBertoli
Copy link
Member Author

Thank you very much @rikukissa @kentcdodds for your feedback.

I confirm that wrapping the current components, or adding a new component in the middle of the tree, makes the output a bit noisy.

For example:

- Snapshot
+ Received

 .c0 {
+  color: red;
+}
+
+.c1 {
   padding: 4em;
   background: papayawhip;
 }
 
-.c1 {
+.c2 {
   font-size: 1.5em;
   -webkit-text-align: center;
   text-align: center;
   color: palevioletred;
 }
 
-<section
+<div
   className="c0"
 >
-  <h1
+  <section
     className="c1"
   >
-    Hello World, this is my first styled component!
-  </h1>
-</section>
+    <h1
+      className="c2"
+    >
+      Hello World, this is my first styled component!
+    </h1>
+  </section>
+</div>

However, the "markup" part of the snapshot would be pretty messed up as well and there's nothing we can really do to avoid it.

So, if this is the trade-off we need to accept in order to get a clean output when the styles change I believe this is the right path to follow.

For example:

- Snapshot
+ Received

 .c0 {
   padding: 4em;
-  background: papayawhip;
+  background: palevioletred;
 }
 
 .c1 {
   font-size: 1.5em;
   -webkit-text-align: center;

Anyway I'm more than happy to hear ideas and feedback, and discuss further to find the optimal solution.

@kentcdodds
Copy link
Member

Yeah, honestly I think that I change the styling more often than I change the markup. And when I change the markup, I often also change the styling anyway and the diff's going to be messy either way. I think this is better 👍 I'll be adding it to jest-glamor-react for sure 👍 Though I think that I'll add it as an opt-in feature then have a breaking change to swap it to opt-out :)

@WickyNilliams
Copy link

Is there some reason this isn't being merged? It appears as if it would be the solution to all my styled-component snapshot woes

@MicheleBertoli
Copy link
Member Author

@WickyNilliams you can use this right now yarn add --dev jest-styled-components@next.
I was waiting for a few more feedback, and I also wanted to find time to write a comprehensive README before shipping the v4.

@WickyNilliams
Copy link

@MicheleBertoli ah didn't realise it had already been published to npm!

Just tried it out. The experience is much better than before - you get valuable insight as to why a snapshot is no longer matching, which was previously hard to determine.

Two thumbs up from me 👍 👍

@mxstbr
Copy link
Member

mxstbr commented Jul 10, 2017

Are we readyyyyyy??

@MicheleBertoli
Copy link
Member Author

Oh yes @mxstbr :)
I spent some time in the last few days improving the code and writing some tests for the website.
I'm waiting a bit before shipping because I want to make sure this change doesn't break anyone's tests.
Today I'm going to write the new README, then I'll be off the grid for a few days and I'll ship the v4 next weekend when I'm back.

@FDiskas
Copy link

FDiskas commented May 21, 2018

Getting something like 🔮

.c0 .CheckboxIcon-q37sjt-0 {
  fill: #b3b3b3;
}

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.

7 participants