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(Gauge echart): displaying column label #23396

Merged
merged 4 commits into from
Apr 8, 2023

Conversation

AkashBoora
Copy link
Contributor

SUMMARY

Changed logic to display the label of field/column. Earlier it was showing field name.

For one dataset i gave label to column 'name', but it was not reflecting in chart. But now the label name is refecting in the chart

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
image

After
image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: [gauge] displaying field name instead of label #16387
  • Required feature flags:
  • Changes UI
  • 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

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #23396 (003ff45) into master (da3791a) will increase coverage by 11.44%.
The diff coverage is 61.97%.

❗ Current head 003ff45 differs from pull request most recent head 54663bd. Consider uploading reports for the commit 54663bd to get more accurate results

@@             Coverage Diff             @@
##           master   #23396       +/-   ##
===========================================
+ Coverage   56.27%   67.71%   +11.44%     
===========================================
  Files        1907     1918       +11     
  Lines       73495    74157      +662     
  Branches     7977     8053       +76     
===========================================
+ Hits        41356    50219     +8863     
+ Misses      30091    21885     -8206     
- Partials     2048     2053        +5     
Flag Coverage Δ
javascript 53.90% <61.97%> (+0.09%) ⬆️

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

Impacted Files Coverage Δ
...t-ui-chart-controls/src/shared-controls/mixins.tsx 16.66% <ø> (ø)
.../packages/superset-ui-core/src/chart/types/Base.ts 100.00% <ø> (ø)
...s/superset-ui-core/src/components/SafeMarkdown.tsx 66.66% <ø> (ø)
...ackages/superset-ui-core/src/query/types/Filter.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-country-map/src/countries.ts 100.00% <ø> (ø)
...plugins/legacy-plugin-chart-heatmap/src/Heatmap.js 0.00% <0.00%> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
...plugins/legacy-plugin-chart-world-map/src/index.js 66.66% <ø> (ø)
...tend/plugins/legacy-preset-chart-nvd3/src/utils.js 15.92% <ø> (ø)
... and 185 more

... and 372 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@AkashBoora
Copy link
Contributor Author

@sfirke could you please review this pr!

@sfirke
Copy link
Member

sfirke commented Mar 23, 2023

@AkashBoora I don't have any permissions on this repository, I just comment and try to help things along. I also don't know most of the codebase. You could try posting in the #contributing channel in the Slack chat asking for a review on this. Looks like a good PR to me though and I'm grateful that you're improving Superset 🙏

@AkashBoora
Copy link
Contributor Author

@villebro could you please review this!

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.

Can you check what happens if you don't have a verbose name defined for your column? I believe my change should address this edge case.

@pull-request-size pull-request-size bot added size/XS and removed size/S labels Apr 7, 2023
@AkashBoora
Copy link
Contributor Author

hello @villebro , yeah the edge case which you have mentioned may not covered by my code. thanks for correcting me :)

@AkashBoora AkashBoora requested a review from villebro April 7, 2023 18:51
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

@villebro
Copy link
Member

villebro commented Apr 8, 2023

@AkashBoora functional changes look good, but I believe there's some linting issues that need to be fixed.

@AkashBoora AkashBoora requested a review from villebro April 8, 2023 09:34
@villebro villebro merged commit b613167 into apache:master Apr 8, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.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/XS 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants