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: chart keys in MultiLineViz #12937

Merged
merged 1 commit into from
Feb 23, 2021
Merged

fix: chart keys in MultiLineViz #12937

merged 1 commit into from
Feb 23, 2021

Conversation

rwspielman
Copy link
Contributor

SUMMARY

fixing multi-line viz issue of incorrectly formatted text for chart key when single line chart is used as a data

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before
BeforeMultiLinVizFix

After
AfterMultiLineVizFix

TEST PLAN

Observe correctly formatted key for data in API call for both multi-line charts and single-line charts as a chart source

ADDITIONAL INFORMATION

@rwspielman rwspielman changed the title [fix] chart keys in MultiLineViz fix: chart keys in MultiLineViz Feb 4, 2021
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.

A minor suggestion, let me know what you think

superset/viz.py Outdated Show resolved Hide resolved
superset/viz.py Outdated
@@ -1453,10 +1453,10 @@ def get_data(self, df: pd.DataFrame) -> VizData:
x_values = [value["x"] for value in series["values"]]
min_x = min(x_values + ([min_x] if min_x is not None else []))
max_x = max(x_values + ([max_x] if max_x is not None else []))

series_keys = series["key"] if isinstance(series["key"], (list, tuple)) else [series["key"]]
Copy link
Contributor Author

@rwspielman rwspielman Feb 4, 2021

Choose a reason for hiding this comment

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

looks like they come in as tuple or list or str (looking at viz_type="line") and defaults to an empty list if empty list or tuple or empty str on line 1451.

I don't want to change much as I'm not very familiar with the codebase yet so sorry this is minimal effort. Hopefully, at some point, I can come back and help maintain this

@codecov-io
Copy link

codecov-io commented Feb 4, 2021

Codecov Report

Merging #12937 (863536f) into master (77093a8) will increase coverage by 0.16%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12937      +/-   ##
==========================================
+ Coverage   68.98%   69.14%   +0.16%     
==========================================
  Files        1025      494     -531     
  Lines       48767    32606   -16161     
  Branches     5241        0    -5241     
==========================================
- Hits        33641    22547   -11094     
+ Misses      14989    10059    -4930     
+ Partials      137        0     -137     
Flag Coverage Δ
cypress ?
javascript ?
python 69.14% <0.00%> (+1.52%) ⬆️

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

Impacted Files Coverage Δ
superset/viz.py 65.12% <0.00%> (+6.07%) ⬆️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/views/database/views.py 62.69% <0.00%> (-24.88%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/sql_validators/base.py 93.33% <0.00%> (-6.67%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
superset/utils/decorators.py 94.44% <0.00%> (-5.56%) ⬇️
superset/views/database/forms.py 83.33% <0.00%> (-5.56%) ⬇️
superset/dataframe.py 94.73% <0.00%> (-5.27%) ⬇️
... and 589 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 77093a8...863536f. Read the comment docs.

@rwspielman rwspielman closed this Feb 4, 2021
@rwspielman rwspielman reopened this Feb 4, 2021
@rwspielman
Copy link
Contributor Author

sorry - was making this into a single commit

@junlincc junlincc added viz:charts:multiline Related to the Multiline chart new:contributor The author is a new contributor labels Feb 5, 2021
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, sorry for dropping the ball on this one!

@villebro villebro closed this Feb 23, 2021
@villebro villebro reopened this Feb 23, 2021
@villebro villebro merged commit e37c2bf into apache:master Feb 23, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 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 new:contributor The author is a new contributor size/XS viz:charts:multiline Related to the Multiline chart 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[multi-chart] Legend Keys are displaying incorrectly
5 participants