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

Fix toBeVisible ignoring Details element #184

Merged
merged 12 commits into from Jan 22, 2020
Merged

Fix toBeVisible ignoring Details element #184

merged 12 commits into from Jan 22, 2020

Conversation

lourenci
Copy link
Contributor

@lourenci lourenci commented Jan 17, 2020

What:

toBeVisible is ignoring an element inside a not opened <details />.

E.g. This div should be reported as a not visible element.

<details>
  <summary>Title</summary>
  <div>This div is not visible</div>
</details>

CodeSandbox

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

<summary /> is visible whether the <details /> is opened or not, this is not resolved in this PR yet.

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #184 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8404ff...b14120c. Read the comment docs.

Copy link
Member

@eps1lon eps1lon left a 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.

@eps1lon eps1lon added the bug Something isn't working label Jan 17, 2020
Copy link
Member

@gnapse gnapse left a 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.

src/__tests__/to-be-visible.js Outdated Show resolved Hide resolved
src/to-be-visible.js Outdated Show resolved Hide resolved
src/to-be-visible.js Outdated Show resolved Hide resolved
@lourenci
Copy link
Contributor Author

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.

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')

@gnapse
Copy link
Member

gnapse commented Jan 18, 2020

@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.

@lourenci
Copy link
Contributor Author

I'm not sure about how we can handle the summary element, it's always visible except when some parent has a [hidden]. I can't think of anything better than that right now.

If you're ok with that, the PR is ok to be merged.

Thanks for the library.

@lourenci lourenci marked this pull request as ready for review January 20, 2020 20:39
@gnapse
Copy link
Member

gnapse commented Jan 20, 2020

I'm not sure about how we can handle the summary element, it's always visible except when some parent has a [hidden]. I can't think of anything better than that right now.

I believe the summary element follows the rule of any other element. If it's <summary hidden> for example, it will be hidden, regardless of wether the parent details is open or not.

Will review and will let you know.

Copy link
Member

@gnapse gnapse left a 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
Content

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.

@lourenci
Copy link
Contributor Author

Nested details elements

It's already ok. I've just created a context for that.

Deeply nested elements inside the details

It's already ok. I've just created a context for that.

@lourenci lourenci requested a review from gnapse January 20, 2020 22:52
Copy link
Member

@gnapse gnapse left a 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.

@gnapse
Copy link
Member

gnapse commented Jan 21, 2020

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).

@gnapse gnapse merged commit e4d61c2 into testing-library:master Jan 22, 2020
@gnapse
Copy link
Member

gnapse commented Jan 22, 2020

🎉 This PR is included in version 5.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lourenci lourenci deleted the fix-to-be-visible branch January 22, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants