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: Add ByTextContent query #303

Closed
wants to merge 2 commits into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 1, 2019

This is a mix of RFC, feature request and personal experience when writing tests. Maybe I'm missing something obvious about getByText but I do not find this query useful in its current state.

What:

Add *ByTextContent query

Why:

Simpler, more user centric API for finding elements with a given textContent

How:

Simply iterate over all elements matching the given selector and comparing with textContent (exact).

Checklist:

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

I think getNodeText is currently overly complex which can lead to surprising behavior in *ByText queries.

Consider the following button: <button><span>Button</span></button> or a bit more convoluted <button><span>But</span><span>ton</span></button>. Both of these have a textContent of Button but either getByText returns the span for the former case or null for the latter case.

I suspect that the current implementation tries to avoid including wrapper divs in e.g. <div class="wrapper"><p>Text</p></div>. I understood those query helpers as a11y first helpers which means it would be sufficient to match textContent and then let the user define meaningful selectors.

This would encourage thinking about using more semantic elements e.g. if getByTextContent('Text') returns multiple spans and divs with the same content one of those should be made a paragraph or given an aria role etc.

Intuitively a user centric approach to testing what happens when the user clicks the button that says "Click me" would just query for getByTextContent('Click me', 'button'). The second argument would be a selector. No normalization config, no exact option etc. All of this can be expressed via TextMatch.

It's IMO ok to fail if surrounding whitespaces changes. Whitespace can change how the user perceives the text (see css whitespace) and the test should reflect that. In a perfect world jsdom could implement innerText and we could use that (it includes layout, whitespace trimming etc).

@eps1lon eps1lon added the enhancement New feature or request label Jul 1, 2019
@kentcdodds
Copy link
Member

Thanks for this @eps1lon, but it's pretty unlikely that this will happen. I'm SUPER resistant to making two ways to do the same thing and while it appears that this may be better than the current implementation of getByText, using straight-up textContent means that the container would match pretty much any getByTextContent query. This is why the current implementation of getByText is so complex.

If you do happen to have one of the edge cases as you described, then you can actually make it work with getByText pretty easily:

// <div><button><span>But</span><span>ton</span></button></div>
getByText(div, (text, el) => /button/i.test(el.textContent)), {selector: 'button'})

For this reason, I don't think that we'll do this.

@eps1lon
Copy link
Member Author

eps1lon commented Jul 1, 2019

Completely understandable. Would you mind leaving this open a bit longer in case others would like to comment?

So far I think a .textContent matcher would be more appropriate with the recommendation to chain this of other selectors. At least so far I haven't encountered cases where I would want to only match Hello, in <p>Hello, <em>World!</em>. It's currently the only API that still feels like testing implementation details i.e. reading getByText(div, (text, el) => /button/i.test(el.textContent)), {selector: 'button'}) is something I would immediately try to rewrite but so far queries don't naturally chain of of on another (mostly because they don't need to).

@kentcdodds
Copy link
Member

Yeah, it's understandably less than optimal. The challenge is that we're trying to work with DOM nodes without actually working with DOM nodes because the user knows nothing about DOM nodes. It's definitely one of the trade-offs we're using to make it at all testable 😬 Not sure how to improve things without introducing more problems.

@bcarroll22
Copy link

@eps1lon another thing to keep in mind too is that even if this doesn't get merged/accepted, it's pretty easy to add custom queries that you and your team find useful. That way if you prefer this over the current matchers, you can encourage the team to use the custom one instead.

For example, if you're using React Testing Library, you could use the queries option along side the buildQueries helper to add this to a custom render.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jul 1, 2019

I think this is essentially a dupe of #201

The big issue is how to prioritize the matches so that you get the innermost matching element instead of outermost. For example, the button instead of document.body for a case like this:

<body>
  <div>
    <button>Foo</button>
  </div>
</body>

@kentcdodds kentcdodds closed this Jul 1, 2019
@eps1lon eps1lon deleted the feat/byTextContent branch July 2, 2019 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants