Skip to content

Conversation

bewildergeist
Copy link
Contributor

What:
Minor adjustments to the README:

  • A typo in a variable name
  • A few copy-paste errors in variable names
  • Changing the order of a couple of CSS properties to make it clear that the assertion fails because of a different value, not a different order of properties

Why:
Just makes the docs easier to read.

How:
N/A

Checklist:

  • Documentation
  • Tests N/A
  • Updated Type Definitions N/A
  • Ready to be merged
  • Added myself to contributors table N/A

Dan Okkels Brendstrup added 4 commits October 31, 2018 15:13
Makes it easier to grok when quickly browsing the examples.
Makes it clear that what makes the assertion fail is that the color
property is different, not that the order of properties is different.
@gnapse
Copy link
Member

gnapse commented Oct 31, 2018

Hi, thanks!

One thing though. I do not get why your changes regarding css properties order are consistent with the reason you state here?

Changing the order of a couple of CSS properties to make it clear that the assertion fails because of a different value, not a different order of properties

The css properties were already in the same order in the sample element as in the sample test code. This made it clear that the order is not important. Because even with the same order, the .not.toHaveStyle assertions still pass.

@bewildergeist
Copy link
Contributor Author

@gnapse Yeah, the CSS order thing is a very minor niggle, I'm happy to remove that again. My thinking was:

  1. The sample element specifies the order display: none; color: red;
  2. The second assertion specifies that color: red; display: none; will be true, thus already proving that property order doesn't matter (as anyone who knows CSS will know, of course).
  3. Then the third assertion should specify that color: blue; display: none; will be false and show the properties in the same order as the previous assertion, to make it immediately apparent to the reader that red/blue is the only difference between those two assertions.

The fact that the order of properties is inconsequential should basically be a given, so I just thought we should lower the "cognitive overhead" of telling those two assertions apart. But as I said, definitely a very minor point, happy to revert ☺️

Copy link
Member

@gnapse gnapse left a comment

Choose a reason for hiding this comment

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

Nah, forget it. Let's follow your lead on this. Thanks!

@gnapse gnapse merged commit 84a68c1 into testing-library:master Oct 31, 2018
@bewildergeist bewildergeist deleted the pr/fix-readme-typos branch October 31, 2018 20:01
@gnapse
Copy link
Member

gnapse commented Nov 6, 2018

🎉 This PR is included in version 2.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gnapse gnapse added the released label Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants