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: Add baseElement to render return type #151

Merged
merged 2 commits into from
Aug 3, 2018
Merged

fix: Add baseElement to render return type #151

merged 2 commits into from
Aug 3, 2018

Conversation

gnapse
Copy link
Member

@gnapse gnapse commented Aug 1, 2018

What:

Add baseElement to render return type.

Why:

Because without it TypeScript code using this library won't be able to use baseElement, which is crucial to testing react portals, for example.

How:

By adding the baseElement property to the type definitions.

Checklist:

  • Ready to be merged

@gnapse
Copy link
Member Author

gnapse commented Aug 1, 2018

Actually I now think there's an inconsistency in the type definitions. The render function is documented to optionally receive a container: HTMLElement, but it returns this very container (or a newly created one if none is provided), and the one returned is documented to be of type HTMLDivElement. What if the container that was passed in is not a div?

The above concern also applies to the baseElement. It is documented as an optional option of type HTMLElement, so the type of the returned one should also be HTMLElement, not HTMLDivElement.

@pbomb
Copy link
Collaborator

pbomb commented Aug 2, 2018

I agree that baseElement and container should both be typed as HTMLElement instead of HTMLDivElement. I also wouldn't expect this change to impact hardly anyone as it seems rare that one would need to access div-specific properties. I'm not even sure what is div-specific, if anything at all.

@gnapse
Copy link
Member Author

gnapse commented Aug 3, 2018

Any ideas on when this can be merged? I wonder why the codecov task above does not finish.

@kentcdodds
Copy link
Member

I've been having trouble with codecov's status report for a month or two. I'm not sure what the problem is and haven't had time to investigate :-/

@kentcdodds kentcdodds merged commit 9667967 into testing-library:master Aug 3, 2018
@kentcdodds
Copy link
Member

🎉 This PR is included in version 4.1.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gnapse gnapse deleted the patch-2 branch August 3, 2018 15:48
@gnapse
Copy link
Member Author

gnapse commented Aug 3, 2018

No problem, good to know. Thanks!

julienw pushed a commit to julienw/react-testing-library that referenced this pull request Dec 20, 2018
<!--
Thanks for your interest in the project. Bugs filed and PRs submitted are appreciated!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

If you're new to contributing to open source projects, you might find this free
video course helpful: http://kcd.im/pull-request

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->

**What**:

*Unofficially* support node 6 by changing `Object.entries` to `Object.keys`

<!-- Why are these changes necessary? -->

**Why**:

I opened a [PR](jestjs/jest#7346) to jest that adds an example for testing with `react-testing-library`.

jest runs it's test suite against node 6 (including the example I added), which is failing because of the call to `Object.entries`.

Switching to `Object.keys` should make the test pass so the PR can be merged 😸 

See tweet from @kentcdodds: https://twitter.com/kentcdodds/status/1060735136172466176

<!-- How were these changes implemented? -->

**How**:

With code 😸 

<!-- Have you done all of these things?  -->

**Checklist**:

<!-- add "N/A" to the end of each line that's irrelevant to your changes -->

<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->

- [ ] Documentation N/A
- [ ] Tests N/A (I ran them... although not in node 6 😅 because the tests use `async`)
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [x] Added myself to contributors table <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants