-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix(Mixed Timeseries Chart): Custom Metric Label #17649
Conversation
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. Thanks for the fix!
Hi @Yahyakiani , Thanks for the fix. Could you mind fixing the lint? |
Is anything else required on this issue ? |
@Yahyakiani these linting errors are blocking CI: |
@villebro Actually I ran lint locally multiple times. It seems to run without error and on saving the file there are no changes. |
@Yahyakiani ok let me checkout your PR locally and see if I can repro the problem |
@Yahyakiani hey, I think you may need to lint all files instead of one file under superset-frontend. |
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.
The issues aren't directly "linting" errors per se, but actually type errors. See my comment below.
superset-frontend/packages/superset-ui-core/src/query/types/QueryFormData.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #17649 +/- ##
==========================================
- Coverage 68.71% 67.87% -0.85%
==========================================
Files 1596 1652 +56
Lines 65224 66263 +1039
Branches 6950 7122 +172
==========================================
+ Hits 44817 44973 +156
- Misses 18522 19393 +871
- Partials 1885 1897 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Fix seems mostly correct, but one question
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
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, thanks for the fix and diligently iterating on this!
SUMMARY
Bug Fix
This PR fixes the problem in which custom metric name is shown in legend instead of custom metric label inside Mixed Time series charts. Reference: Issue #17586
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION