-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Refactor Guide
tests to @testing-library/react
#43380
Conversation
Size Change: +5 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
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.
Love seeing this sort of refactors, the tests have gained a lot more value!
This also uncovers a bug: when we render with no pages, we attempt to focus the guide container, however it doesn't exist because we don't render it if no pages exist. This PR fixes that bug by changing the component to focus only if the guide container exists.
Amazing!
* | ||
* because the event sent has a `keyCode` of `0`. | ||
* | ||
* To fix this, we'll need to update the Modal component to work with `KeyboardEvent.code`. |
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.
eb8d9fc
to
f433923
Compare
Thanks a bunch, @ciampo! I've rebased after it landed and updated the tests to use This is now ready for another look. |
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.
LGTM 🚀
Co-authored-by: Marco Ciampini <[email protected]>
What?
We've recently started refactoring
enzyme
tests to@testing-library/react
.This PR refactors the
<Guide />
component tests fromenzyme
to@testing-library/react
.Why?
@testing-library/react
provides a better way to write tests for accessible components that is closer to the way the user experiences them.How?
We're straightforwardly replacing
enzyme
tests with@testing-library/react
ones, usingjest-dom
matchers screen queries.This also uncovers a bug: when we render
<Guide />
with no pages, we attempt to focus the guide container, however it doesn't exist because we don't render it if no pages exist. This PR fixes that bug by changing the component to focus only if the guide container exists.Testing Instructions
npm run test:unit packages/components/src/guide