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 unenclosed inner text for details elements in to be visible #396

Conversation

kschow
Copy link
Contributor

@kschow kschow commented Aug 23, 2021

What:

Supporting unenclosed inner text inside <details /> elements for toBeVisible(). See #395

Why:

Unenclosed inner text is valid for <details /> elements and should therefore be supported.

How:

Updated the isAttributeVisible function. In the case where the inner text is unenclosed there is no "previousElement" so defaults to only check the open attribute of the details element itself. Otherwise, it follows the same logic as before.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions "N/A"
  • Ready to be merged

@@ -49,7 +49,6 @@ clear to read and to maintain.
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->


Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this, and the other formatting change might have been automatic, I didn't do them intentionally but I can try to revert those changes if necessary.

})
})

describe('with nested details (unenclosed outer, enclosed inner)', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This set of tests might be a bit redundant, I added them because I wasn't sure how the unenclosed and enclosed details would interact. But thinking it over, there really shouldn't be a significant difference as opposed to the previous nested tests (which these are based off).

@feedmypixel
Copy link
Contributor

I've just come across this issue when testing details with "@testing-library/jest-dom": "5.16.2". This PR looks a little stale? any help needed?

@kschow
Copy link
Contributor Author

kschow commented Mar 30, 2022 via email

@feedmypixel
Copy link
Contributor

Feel free to do what you think is best here.

I have rebased main. All linting and testing is passing, can you add me as a collaborator on https://github.com/kschow/jest-dom/ and I will raise a PR to merge into your branch. Once you have merged that into yours that then should kick the status required checks here and we will be moving forward

…etails-elements-in-toBeVisible-main-rebased

Support unenclosed inner text for details elements in to be visible main rebased
@feedmypixel
Copy link
Contributor

Ok thats been raised on your repo and merged into your branch - i'll leave it up to you but thats your original PR with an updated main - its also kicked off the status checks here

@kschow
Copy link
Contributor Author

kschow commented Mar 30, 2022

I think at this point we're waiting on a maintainer to take a look at this and approve it.

@feedmypixel
Copy link
Contributor

feedmypixel commented Mar 31, 2022

I think at this point we're waiting on a maintainer to take a look at this and approve it.

It looks like there is one task to complete - but also may be worth dropping a friendly hello to @gnapse to see if this can be progressed. I'll watch with keen 👀 to see what happens with this 👍

@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #396 (fc80f89) into main (6988a67) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #396   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          626       630    +4     
  Branches       233       234    +1     
=========================================
+ Hits           626       630    +4     
Impacted Files Coverage Δ
src/to-be-visible.js 100.00% <100.00%> (ø)

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 6988a67...fc80f89. Read the comment docs.

@gnapse gnapse merged commit af18453 into testing-library:main Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

🎉 This PR is included in version 5.16.4 🎉

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.

None yet

3 participants