-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Remove highcharts dependency pt. 1 #15970
Conversation
Signed-off-by: Frances Thai <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
d3
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
I'll update the changelog in this PR as well, as well as update the old tests that reference old charts 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.
I'm JS illiterate, but there are some linter warnings that we should address. It also seemed like we might have accidentally added an unnecessary/unwanted line in the package file.
@@ -1,5 +1,5 @@ | |||
/** | |||
* Copyright 2021 The Vitess Authors. | |||
* Copyright 2024 The Vitess Authors. |
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.
I think this is fine, but we don't typically NEED to change the year when updating an existing file.
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.
+1
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.
Ah, that's good to know. I thought someone had told me that we needed to in the past (it may have just been for net-new files). I won't update them going forwards.
Signed-off-by: Frances Thai <[email protected]>
@mattlord I always intended to fix the linter issues 😄 my ask for review is for the structure and usage of the code. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15970 +/- ##
==========================================
+ Coverage 68.23% 68.25% +0.02%
==========================================
Files 1541 1541
Lines 197187 197168 -19
==========================================
+ Hits 134546 134576 +30
+ Misses 62641 62592 -49 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Frances Thai <[email protected]>
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.
From a pass at it, it LGTM
Signed-off-by: Frances Thai <[email protected]>
react changes look good to merge 🚀 |
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
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.
⚡️
@@ -13,9 +13,9 @@ | |||
"@types/node": "^16.11.7", | |||
"@types/react-router-dom": "^5.3.3", | |||
"classnames": "^2.3.2", | |||
"d3": "^7.9.0", |
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.
Maybe add @types/d3
too?
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.
✨ absolutely, TY
Signed-off-by: Frances Thai <[email protected]>
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! ❤️
web/vtadmin/src/components/charts/WorkflowStreamsLagChart.test.tsx
Outdated
Show resolved
Hide resolved
web/vtadmin/src/components/charts/WorkflowStreamsLagChart.test.tsx
Outdated
Show resolved
Hide resolved
….tsx Signed-off-by: Frances Thai <[email protected]>
web/vtadmin/src/components/charts/WorkflowStreamsLagChart.test.tsx
Outdated
Show resolved
Hide resolved
….tsx Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Signed-off-by: Frances Thai <[email protected]>
Description
This PR begins replacing
highcharts
withd3
. We need to removehighcharts
dependency because it does not meet licensing requirements for Vitess.This PR removes the highcharts charts, and implements a bare minimum graph.
New graph:
Old graph:
Please see the "issue" section for what features are missing in this PR, compared to the old highcharts graphs.
Related Issue(s)
This PR addresses the following checkboxes of #16032 :
Remove highcharts
Feature parity
Checklist
Deployment Notes
We won't reach parity with the highcharts in this PR. There will be subsequent PRs that implement the other features - the priority of this PR is to remove highcharts.