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

feat(getByText): add ignore option which defaults to 'script' #109

Merged
merged 1 commit into from Oct 2, 2018

Conversation

kentcdodds
Copy link
Member

What: add ignore option to *ByText which defaults to 'script'

Why:

In our app at PayPal we're testing with Cypress and our localization content is set in a script tag on the page when it's rendered by the server. When using getByText with cypress-testing-library, we were getting that script tag before the app had a chance to render.

I'm pretty confident that 99.999% of the time people wont want to get a script tag with getByText, so the default makes sense to me.

How:

Added another layer of filtering to queryAllByText to filter nodes which return true for .matches(ignore). Feel pretty flexible. Happy that DOM API exists!

Checklist:

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

@codecov
Copy link

codecov bot commented Oct 2, 2018

Codecov Report

Merging #109 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #109   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         184    185    +1     
  Branches       50     51    +1     
=====================================
+ Hits          184    185    +1
Impacted Files Coverage Δ
src/queries.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c07657d...1ae1af6. Read the comment docs.

@kentcdodds
Copy link
Member Author

More explanation here: https://www.youtube.com/watch?v=LYB0SEYaBlE

@kentcdodds kentcdodds merged commit 2536924 into master Oct 2, 2018
@kentcdodds kentcdodds deleted the pr/ignore-script-tag branch October 2, 2018 21:42
exact = true,
collapseWhitespace = true,
trim = true,
ignore = 'script',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think script and style would be a better default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll add that!

Copy link
Collaborator

Choose a reason for hiding this comment

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

But I'm just a couple minutes late...

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries. I just pushed an update directly to master. I figured the overhead would be unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great 👍

@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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