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
Fix toBeVisible ignoring Details element #184
Conversation
Codecov Report
@@ Coverage Diff @@
## master #184 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 235 235
Branches 57 57
=====================================
Hits 235 235 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for recognizing the issue and working on a fix.
I wasn't aware of how different the details
element works with regards to visibility. It seems like neither display
nor visibility
control if the content is displayed or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. We had not thought of this.
Besides the comment I added in this review, I wanted to ask you something else:
Will this work consistently when the user has already interactively clicked on the summary to toggle the visibility of the inner div? If so, I'd like to see a test that does not add the open
attribute manually to the details
element, but that starts with a closed details element, then programmatically clicks on the summary, and then the matcher says it's visible.
Overall I want to make sure this works not just to assert the initial state of the details panel visibility status, but that if the panel is interactively toggled closed and open after its initial render, the results will be consistent.
Hi, @gnapse. I think this makes sense. How can we organize this specs? What do you think about something like that: describe('.toBeVisible')
it('returns the visibility of an element')
//the actual specs
describe('with a <details /> element')
describe('when the details is opened')
it('returns true')
describe('when the user clicks on the summary')
it('returns false')
describe('when the details is not opened')
it('returns false')
describe('when the user clicks on the summary')
it('returns true') |
@lourenci certainly! I'm more than open to begin splitting more our tests to cover the different scenarios separately. So go ahead with what you propose. |
I'm not sure about how we can handle the If you're ok with that, the PR is ok to be merged. Thanks for the library. |
I believe the Will review and will let you know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested details
elements
I'm now taking a more in-depth look at this, and I wonder if this strategy works for nested details elements. I could not easily try all this out because I'm not sure if I can switch locally to your branch, as it is in your repo. I may clone it to try this out, but in the mean time I ask you if this would work?
Take for instance:
<details>
<summary>First</summary>
<details>
<summary>Second</summary>
<div>Content</div>
</details>
</details>
Which looks like this:
First
Second
You can expand both, then collapse only the root details. The innermost div
element is not visible, even when its parent details
element is open.
Deeply nested elements inside the details
Also, this must work for more deeply nested elements inside any details, not just direct child elements. Take the example below:
<details>
<summary>First</summary>
<div>
Content <small>123</small>
</div>
</details>
The <small>123</small>
element should also be detected as not visible when the details
is not open. This last case I'm more confident that it works under your approach, but not the previous case of nested details
. Can you confirm?
Overall, even if these two cases already work, I'd request you to add tests for them, to reflect in the tests that they do work, and to prevent future regressions.
It's already ok. I've just created a context for that.
It's already ok. I've just created a context for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, sorry, I missed that you had already even added those tests. My apologies.
I'm deferring merging this a bit, in an attempt to make sure that our next release v5.0.1 is the one with fixed types support (see DefinitelyTyped/DefinitelyTyped#37792). |
🎉 This PR is included in version 5.0.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What:
toBeVisible
is ignoring an element inside a not opened<details />
.E.g. This div should be reported as a not visible element.
CodeSandbox
Checklist:
<summary />
is visible whether the<details />
is opened or not, this is not resolved in this PR yet.