Update chartjs to latest (v4)#1013
Update chartjs to latest (v4)#1013ericboucher merged 14 commits intoaqualinkorg:masterfrom yaodingyd:chartjs-issue
Conversation
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@yaodingyd you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Looks great to me overall. It's always nice to see cleaning up of tech debt and staying on top of dependency updates. The only things I caught were relatively minor.
Ryan Lester <@buu700>
Reviewed with ❤️ by PullRequest
| } from 'react'; | ||
| import { Line } from 'react-chartjs-2'; | ||
| import type { ChartTooltipModel } from 'chart.js'; | ||
| // import { Line } from 'react-chartjs-2'; |
There was a problem hiding this comment.
Is the plan for these to be uncommented and added back, or can they be removed? It may be worth adding a TODO comment to clarify when they can be uncommented, as well as a comment to clarify why the types below were changed to any.
If it's just a temporary workaround for a third-party issue, you could temporarily include something like this:
// Temporarily defining these as `any` for XYZ reason
type Line = any;
type TooltipModel = any;
(Or alternatively, you could define the types to be more specific than any.)
Also, when/if the imports are uncommented, if only the type is needed for Line, you could use import type as with TooltipModel.
The same general questions and comments also apply to the commented imports in the other files.
🔹 Dead Code (Nice to have)
Ryan Lester <@buu700>
There was a problem hiding this comment.
This pull request would normally not be reviewed by PullRequest because the pull request is a draft; however since a review was already requested, it is currently being reviewed by the PullRequest network. If you would like to cancel it, you can do so manually from the PullRequest dashboard.
| import { Line } from 'react-chartjs-2'; | ||
| import 'chart.js/auto'; | ||
| import { Tick } from 'chart.js'; | ||
| import 'chartjs-adapter-date-fns'; |
There was a problem hiding this comment.
Looks like importing this like that is causing issues when running the tests.
We could try to import it in another file like you did for the annotation plugin, or maybe mock it during tests.
|
@yaodingyd great work, thanks! I noticed that we still have a "ghost" point when hovering on a survey. Eg. on site 3197. See the blue point over 
|
|
@ericboucher can you take another look? I think the ghost hover issue is fixed Screen.Recording.2024-08-31.at.10.45.47.PM.mov |
ericboucher
left a comment
There was a problem hiding this comment.
Thanks @yaodingyd, great work!!
fix #994
/claim #994
This is to update chartjs to latest V4 and related dependencies to latest. There are a lot breaking changes from V2 -> V4, includes:
there are some remaining type issues (all of them are related to using
anyto avoid typecheck error) but it is in a good state to review