Skip to content

Ignore extra whitespace in node texts #21

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Apr 11, 2018
Merged

Ignore extra whitespace in node texts #21

merged 6 commits into from
Apr 11, 2018

Conversation

gnapse
Copy link
Member

@gnapse gnapse commented Apr 11, 2018

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Almost everything looks great!

src/queries.js Outdated
@@ -173,6 +171,7 @@ export {
queryByPlaceholderText,
getByPlaceholderText,
queryByText,
getText,
Copy link
Member

Choose a reason for hiding this comment

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

This file should only expose queries. Other utilities should live in other files. Could you move getText to its own file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Also, I wonder if we should publicly call it getNodeText instead?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We can rename it to getNodeText 👍

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Super duper. Thanks!

README.md Outdated
// Hello
// World !
// </div>
const text = getNodeText(container.querySelector('div')) // "Hello World!"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be: "Hello World !" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, good catch!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Hmmm... Looks like these changes broke a test 🤔

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

This looks unrelated, and perhaps there's a brittle test in the wait-for-element part. I just ran npm run test:update locally and it all passes with full coverage. Plus a change to the README breaking a test makes no sense.

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

BTW (unrelated but) have you considered adding npm run test:update to be run in a pre-push hook in your projects?

@kentcdodds
Copy link
Member

I've restarted the build. Yes, that's odd. Hmmm...

BTW (unrelated but) have you considered adding npm run test:update to be run in a pre-push hook in your projects?

I have not! Hmmm... I wonder if I could make it interactive. If there are snapshot differences, ask the user if they'd like to cancel the commit or update them... Not sure that'd be possible with lint-staged though...

@gnapse
Copy link
Member Author

gnapse commented Apr 11, 2018

The restarted build passed! 😱

@kentcdodds
Copy link
Member

Uh oh... We should probably look into that.

@kentcdodds kentcdodds merged commit 2273f03 into testing-library:master Apr 11, 2018
@gnapse gnapse deleted the pr/ignore-extra-whitespace branch April 11, 2018 18:53
alexkrolick pushed a commit to alexkrolick/dom-testing-library that referenced this pull request Sep 13, 2018
* added waitForExpect with test

* added typescript and simplified version of the waitForExpect, used and exports its typings

* added initial notes about waitForExpect

* fixed styling

* minor stylistic change

* updated tests to remove the nesting

* updated readme

* fixed d .md syntax

* improved style

Closes testing-library#21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants