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

2149 - add canvas mock #2167

Merged
merged 2 commits into from
Oct 14, 2022
Merged

Conversation

yasirazgar
Copy link
Contributor

Description

Added canvas mock package to fix Chart.spec.jsx
Reference: hustcc/jest-canvas-mock#2

Corresponding Issue

#2149

Screenshots


Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!

Copy link
Member

@julianguyen julianguyen 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 taking this on! 🎉 Some feedback and we should be good to go!

@@ -10060,6 +10065,14 @@ iterate-value@^1.0.2:
es-get-iterator "^1.0.2"
iterate-iterator "^1.0.1"

jest-canvas-mock@^2.4.0:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be explicitly including jest-canvas-mock in our package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added jest-canvas-mock to package.json, I believe we still need this change as well, right ?

@@ -2,6 +2,7 @@
import React from 'react';
import { render } from '@testing-library/react';
import { Chart } from 'components/Chart/index';
import 'jest-canvas-mock';
Copy link
Member

Choose a reason for hiding this comment

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

ESLint raised an import/no-unresolved error. If you run yarn lint:eslint locally you should see that error. While we could make this an exception, I think a better solution would be to include this in our Jest setup.

Check out the docs for this depedendency on how to do that.

We'll want to add here in our /client/package.json file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the dependencies.

Copy link
Member

@julianguyen julianguyen left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks so much for taking this on! 🎉

@julianguyen julianguyen merged commit 5d865a1 into ifmeorg:main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants