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

test(component): refactor collapse component tests #654

Merged

Conversation

bc-alexsaiannyi
Copy link
Contributor

@bc-alexsaiannyi bc-alexsaiannyi commented Nov 24, 2021

What?

This PR is aimed to refactor Collapse component's tests according to best practices. We take into account recommendations from Kent C. Dodds article. For instance, using container for query-ing elements has been removed. Instead, we replace it with screen.

NOTE: userEvent package has been added for switching over fireEvent.

Why?

the changes are based on GitHub issue and PRs will be open for each component separately.

Testing/Proof

The tests have been successfully run locally.

@bc-alexsaiannyi bc-alexsaiannyi marked this pull request as ready for review December 3, 2021 10:32
@bc-alexsaiannyi bc-alexsaiannyi requested review from a team as code owners December 3, 2021 10:32
const icon = trigger.querySelector('svg');
render(CollapseWithStaticTitleMock);

const icon = screen.getByRole('button').querySelector('svg');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍹 Using querySelector to get the SVG is a little concerning. It might mean that we need to add a role to the image for accessibility, potentially along with any additional aria tags 🤔

No action needed for this, just pointing out that if we are using querySelector something might be wrong.

@bc-alexsaiannyi bc-alexsaiannyi merged commit 44f0962 into bigcommerce:master May 10, 2022
bc-juanvasquez pushed a commit to bc-juanvasquez/big-design that referenced this pull request May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants