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

[bubble] Fixing ad-hoc metric labels #5630

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Aug 14, 2018

This PR fixes the bubble chart labels which previously rendered as [object Object] if the metric was an ad-hoc metric.

to: @GabeLoins @graceguo-supercat @michellethomas @mistercrunch @timifasubaa

@codecov-io
Copy link

codecov-io commented Aug 14, 2018

Codecov Report

Merging #5630 into master will increase coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5630      +/-   ##
==========================================
+ Coverage   63.44%   63.49%   +0.04%     
==========================================
  Files         360      360              
  Lines       22890    22890              
  Branches     2548     2551       +3     
==========================================
+ Hits        14523    14533      +10     
+ Misses       8352     8342      -10     
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/visualizations/nvd3_vis.js 8.94% <0%> (ø) ⬆️
superset/connectors/sqla/models.py 78.66% <0%> (+0.76%) ⬆️
superset/db_engine_specs.py 54.19% <0%> (+0.88%) ⬆️

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 2171ffb...0f570e7. Read the comment docs.

Copy link
Contributor

@conglei conglei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non ad-hoc metric field? seems like we should first test if fd.x is an object or it is just a string

s += row(fd.x, formatter(p.x));
s += row(fd.y, formatter(p.y));
s += row(fd.size, formatter(p.size));
s += row(fd.x.label, formatter(p.x));
Copy link
Contributor

@conglei conglei Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about non ad-hoc metric field? seems like we should first test if fd.x is an object or it is just a string. Same thing for the other two.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@conglei only ad-hoc metrics are defined for bubble-charts.

@john-bodley john-bodley force-pushed the john-bodley-bubble-adhoc-metrics branch from 362dd09 to d455983 Compare August 14, 2018 22:37
@john-bodley
Copy link
Member Author

@conglei thanks for bringing this to my attention. I've updated the logic.

Copy link

@graceguo-supercat graceguo-supercat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@john-bodley john-bodley merged commit 46f89f4 into apache:master Aug 15, 2018
john-bodley added a commit to john-bodley/superset that referenced this pull request Aug 15, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants