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

Update documentation for toBeVisible() when using details elements #395

Open
kschow opened this issue Aug 20, 2021 · 2 comments
Open

Update documentation for toBeVisible() when using details elements #395

kschow opened this issue Aug 20, 2021 · 2 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@kschow
Copy link
Contributor

kschow commented Aug 20, 2021

I was working on something using the <details /> element and was looking to test whether the inner text was visible or not. What I found out while looking through the code/tests for toBeVisible() is that:

  1. The inner text needs to be inside another element. e.g.
<details>
    <summary>Title</summary>
    <div>Explanation text</div>
</details>
  1. This is necessary because of the previousElement check in isAttributeVisible
    (element.nodeName === 'DETAILS' && previousElement.nodeName !== 'SUMMARY'
  2. When a <details /> element without enclosed inner text is used in conjunction with React Testing Library, you'll receive TypeError: Cannot read property 'nodeName' of undefined. E.g.
<details>
    <summary>This will not work</summary>
    Unenclosed Inner Text
</details>
expect(getByText('Unenclosed Inner Text')).not.toBeVisible();
  1. While the documentation does mention that toBeVisible() works with <details /> elements with the open attribute, it neither mentions the above or give an example.

Since <details /> elements do not need to have an enclosing element for their inner text, see the first example https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details, it should be clearer that this is necessary. It's probably ok if this acts as it is, I just think it should be clearer that it's explicitly necessary and there should probably be an example for <details /> regardless.

@gnapse
Copy link
Member

gnapse commented Aug 20, 2021

Hi, thanks for your detailed issue report.

I'd even say this is not an issue with the documentation, but with the implementation. It should work with the unenclosed inner text. I'd fix that instead of putting a caveat in the documentation. Let's consider this a bug.

@gnapse gnapse added bug Something isn't working good first issue Good for newcomers labels Aug 20, 2021
@sjarva
Copy link

sjarva commented Dec 4, 2022

Hi! Is this issue closed by #396 or is there still some part that hasn't been implemented? 🤔
I was looking for a good first issue and got excited by this, but to me it seems like this has been done 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants