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

Support textareas with children #344

Merged
merged 1 commit into from Sep 5, 2019

Conversation

@amannn
Copy link
Contributor

commented Aug 22, 2019

What:

Fixes this bug: #343 (comment)

Checklist:

  • Documentation added to the
    docs site N/A
  • Typescript definitions updated N/A
  • Tests
  • Ready to be merged
// The children of a textarea are part of `textContent` as well. We
// need to remove them from the string so we can match it afterwards.
label.querySelectorAll('textarea').forEach(textarea => {
textToMatch = textToMatch.replace(textarea.value, '')

This comment has been minimized.

Copy link
@alexkrolick

alexkrolick Aug 22, 2019

Collaborator

Hmm... What if the label is the same as the text value at that moment?

This comment has been minimized.

Copy link
@timdeschryver

timdeschryver Aug 22, 2019

Member

I think we should be able to use label.childNodes[0].textContent here?

This comment has been minimized.

Copy link
@amannn

amannn Aug 23, 2019

Author Contributor

@alexkrolick replace only replaces the first occurence:

'abcabc'.replace('abc', '') // "abc"

Therefore this should be fine.

@timdeschryver Sorry, I can't follow? Generally I think it's good to avoid childNodes so we support arbitrary nesting for both the textarea and the label. There could be some divs and spans wrapped around for example.

Copy link
Member

left a comment

This is a little odd, but I think I'm ok with this change. I can't think of situations where it would lead to people breaking our goals with testing and it doesn't add anything to the API. It also doesn't really violate the principle of least surprise, so I think I'm good with it. I'd love a 👍 from another maintainer though.

@amannn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

Thanks for the review! Is there another maintainer who wants to have a look at this? Would be great to get this fixed, so I can remove a workaround in my code base 🙂. Thanks!

@kentcdodds kentcdodds merged commit f9714ed into testing-library:master Sep 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kentcdodds

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@all-contributors please add @amannn for code and tests

@allcontributors

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@kentcdodds

I've put up a pull request to add @amannn! 🎉

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

🎉 This PR is included in version 6.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kentcdodds kentcdodds added the released label Sep 5, 2019
@afontcu afontcu referenced this pull request Sep 19, 2019
1 of 4 tasks complete
@amannn amannn changed the title Support textareas with children. Support textareas with children Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.