-
Notifications
You must be signed in to change notification settings - Fork 508
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
(feat): Add react-testing-library to react templates #927
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
4da3770
to
0292873
Compare
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 the contribution @lukasbals !
This covers most of the necessary changes, but there's some unnecessary changes here that will need to be removed, and some best practices that will require some changes. There's also some smaller portions that I need to double-check don't accidentally break things as well and I'll need to do some manual testing of this since we don't yet have E2E tests for tsdx create
. Please see the comments in-line.
src/templates/react.ts
Outdated
'react', | ||
'react-dom', | ||
'@testing-library/react', | ||
'@testing-library/jest-dom', |
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.
Alphabetically, this should actually be at the top; @testing
comes before @types
.
Also, we might need to pin an earlier version of @testing-library/jest-dom
as this might be a Jest 26/TS 3.8+ version which is slated for TSDX v0.16.0... need to look into that
@@ -8,4 +10,13 @@ describe('Thing', () => { | |||
ReactDOM.render(<Thing />, div); | |||
ReactDOM.unmountComponentAtNode(div); | |||
}); |
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.
So we probably don't need the ReactDOM
test anymore with this, but I need to do some more reading on @testing-library/react
internals to see if it covers this usage test (I believe it does, but not 100% sure).
3e7dab7
to
f2b39d1
Compare
Hi @agilgur5. I pushed a fixup PR quite some time ago. Is there any further action required from my side? You mentioned you want to double-check some stuff, what's the state there? No rush just wanted to know how things are. |
Added
react-testing-library
to thereact
and thereact-with-storybook
template. Additionally, updated the readme-files to link to thereact-testing-library
docs and implemented sample tests.