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

add excludeHidden for *ByText queries #1184

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rluvaton
Copy link

@rluvaton rluvaton commented Oct 26, 2022

What: add excludeHidden option for *ByText queries

Why: #196

How: used the already existing isInaccessible function

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@rluvaton
Copy link
Author

Before I start adding tests, what do you think about this?


Cc @kentcdodds

I'll reopen this and I'd be willing to look at a PR for this.
#196 (comment)

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c546d52:

Sandbox Source
react-testing-library-examples Configuration

@kentcdodds
Copy link
Member

Honestly, I think this would be a worthwhile thing in all the queries and eventually should be enabled by default (which of course would be a breaking change).

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Why not use the same name for the option as ByRole does?

Also note that this has considerable perf impact. ByText etc are a faster, less accurate helper and with this change we're making the distinction between By* and ByRole less clear.

@kentcdodds
Copy link
Member

I think that's fair @eps1lon. As I'm no longer maintaining Testing Library, I don't think it's fair for me to make decisions here. I think the best would be to have this supported in all the queries and the "faster, less accurate helper" can be offered by using the option.

@eps1lon
Copy link
Member

eps1lon commented Oct 27, 2022

I'm not fundamentally opposed. But the added dimension due to the naming of the option is a bigger concern for me right now.

@kentcdodds
Copy link
Member

Oh, yeah, I agree we should probably keep the names consistent 👍

@rluvaton
Copy link
Author

rluvaton commented Oct 29, 2022

I used different name to avoid confusion with *ByRole as that hidden: true there means to show hidden as well
While in the *ByText we want to exclude them, and if we use the same configuration we would break our users as currently defaultHidden is false - meaning that we don't return hidden results and which is not what we currently do for *ByText queries...

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