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

fix(Mixed Timeseries Chart): Custom Metric Label #17649

Merged
merged 5 commits into from
Dec 14, 2021

Conversation

Yahyakiani
Copy link
Contributor

@Yahyakiani Yahyakiani commented Dec 4, 2021

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

before
after

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Fixes [mixed time-series] Metric name is shown in legend instead of label #17586
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@Yahyakiani Yahyakiani changed the title fix(Mixed Timeseries Chart): Custom Metric Label #17586 fix(Mixed Timeseries Chart): Custom Metric Label Dec 4, 2021
Copy link
Member

@stephenLYZ stephenLYZ left a 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!

@zhaoyongjie
Copy link
Member

Hi @Yahyakiani , Thanks for the fix. Could you mind fixing the lint?

@Yahyakiani
Copy link
Contributor Author

Hi @Yahyakiani , Thanks for the fix. Could you mind fixing the lint?

Is anything else required on this issue ?

@zhaoyongjie
Copy link
Member

zhaoyongjie commented Dec 8, 2021

Is anything else required on this issue ?

The CI need to be fixed.
image

@Yahyakiani
Copy link
Contributor Author

Is anything else required on this issue ?

The CI need to be fixed. image

What steps could I perform to resolve it? Because I have tried running lint but it seems to make no difference.

@villebro
Copy link
Member

villebro commented Dec 8, 2021

@Yahyakiani these linting errors are blocking CI:
image
Let me know if you need help resolving them

@Yahyakiani
Copy link
Contributor Author

@Yahyakiani these linting errors are blocking CI: image Let me know if you need help resolving them

@villebro Actually I ran lint locally multiple times. It seems to run without error and on saving the file there are no changes.
lint1
eslint

@villebro
Copy link
Member

villebro commented Dec 8, 2021

@Yahyakiani ok let me checkout your PR locally and see if I can repro the problem

@stephenLYZ
Copy link
Member

@Yahyakiani hey, I think you may need to lint all files instead of one file under superset-frontend.

Copy link
Member

@villebro villebro left a 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.

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #17649 (24ee345) into master (4306289) will decrease coverage by 0.84%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Flag Coverage Δ
javascript 56.24% <0.00%> (-1.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...hart-echarts/src/MixedTimeseries/transformProps.ts 11.45% <0.00%> (-0.37%) ⬇️
...rset-ui-core/src/connection/SupersetClientClass.ts 90.76% <0.00%> (-7.54%) ⬇️
...nd/src/explore/components/DataTablesPane/index.tsx 76.92% <0.00%> (-0.75%) ⬇️
...rc/explore/components/ExploreChartHeader/index.jsx 46.57% <0.00%> (-0.65%) ⬇️
...et-frontend/src/components/EditableTitle/index.tsx 73.07% <0.00%> (-0.61%) ⬇️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx 21.34% <0.00%> (-0.50%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 38.53% <0.00%> (-0.36%) ⬇️
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx 67.15% <0.00%> (-0.26%) ⬇️
superset-frontend/src/components/Select/Select.tsx 86.33% <0.00%> (ø)
...uperset-frontend/src/utils/getClientErrorObject.ts 71.87% <0.00%> (ø)
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4306289...24ee345. Read the comment docs.

Copy link
Member

@villebro villebro left a 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

Copy link
Member

@villebro villebro left a 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!

@villebro villebro merged commit 89d0d38 into apache:master Dec 14, 2021
@Yahyakiani Yahyakiani deleted the fix-custom-metric-label branch December 14, 2021 07:05
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mixed time-series] Metric name is shown in legend instead of label
5 participants