Skip to content

Conversation

costaraphael
Copy link
Contributor

@costaraphael costaraphael commented Feb 12, 2020

What:

Updating the ByLabel queries to work with <select> components. It didn't work properly with <select> components because the text from the options is part of the label's textContent property.

Why:

It seems super counter-intuitive that it works really well for other inputs (even <textarea>), but not for <select>. I think having this working properly will make this library an even greater joy to use ❤️

How:

I used the same strategy used for <textarea> components, but instead removing the textContent value from the <select> components from the label's textContent.

Checklist:

  • Documentation added to the (I'm not sure it's needed in this case)
    docs site
  • I've prepared a PR for types targeting (I'm not sure either)
    DefinitelyTyped
  • [ x] Tests
  • [ x] Ready to be merged

If anyone is having this same issue and this fix is not available, you can use this workaround:

getByLabelText(/^Label/)
// instead of
getByLabelText('Label')

I would also like to add that you rule for making front-end testing such a pleasant experience! ❤️

@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 0a4ba3d:

Sandbox Source
competent-agnesi-idoee Configuration

@codecov
Copy link

codecov bot commented Feb 12, 2020

Codecov Report

Merging #451 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #451   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines          385       387    +2     
  Branches        91        91           
=========================================
+ Hits           385       387    +2     

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 cf57dcd...0a4ba3d. Read the comment docs.

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.

I wonder if there's a way to deal with the TextNode directly rather than try and filter out the text content of select and it's options. 🤔 Could you investigate that? Otherwise I'm fine with this for now.

@costaraphael
Copy link
Contributor Author

It would be much better IMO.

I'll have a look at this and come back with my findings 😊

@costaraphael
Copy link
Contributor Author

costaraphael commented Feb 12, 2020

@kentcdodds I tested this and it seems to work fine:

const textToMatch = Array.from(label.childNodes)
  .filter(n => n.nodeName === "#text")
  .map(n => n.data)
  .join('')

There is a hiccup with this approach, though. It limits the usage of inline elements within the label text (like span, b, or i), which would be a breaking change from the current behaviour.

While I'm at this, I was looking at other issues/PRs (mainly this one: #372) and this seems like something desirable from the library POV. I'm not sure if this is the right place to ask this, but what is the reason behind this?

EDIT: replacing n.wholeText with n.data, as the former could cause some weird behaviour at some edge cases

@kentcdodds
Copy link
Member

Sorry for the delay. If you would like to make that update based on your text node version I would be willing to merge that. I'd love a review from someone else watching the repo though :)

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.

After further investigation, I don't think my suggested "solution" will work here. So this looks good. Thank you!

@kentcdodds kentcdodds merged commit a3c5699 into testing-library:master Mar 4, 2020
@kentcdodds
Copy link
Member

🎉 This PR is included in version 6.14.1 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants