feat: Migrates Area chart #23870
feat: Migrates Area chart #23870
Conversation
michael-s-molina
added
risk:breaking-change
michael-s-molina
requested review from
jinghua-qa,
geido,
eschutho,
rusackas,
betodealmeida and
a team
as code owners
Codecov Report
@@ Coverage Diff @@
## master #23870 +/- ##
=======================================
Coverage 68.28% 68.29%
=======================================
Files 1955 1953 -2
Lines 75493 75499 +6
Branches 8220 8220
=======================================
+ Hits 51553 51562 +9
+ Misses 21833 21830 -3
Partials 2107 2107
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
michael-s-molina
force-pushed
the
migrate-area
branch
from
ef3690b
to
7089df3
Compare
| remove_keys = {"contribution", "stacked_style", "x_axis_label"} | ||
| rename_keys = { | ||
| "x_axis_label": "x_axis_title", | ||
| "x_axis_format": "x_axis_time_format", |
There was a problem hiding this comment.
I'm surprised this has the term time in it given that I thought part of the broader EChart migration was to support time agnostic, i.e., generic, charts.
There was a problem hiding this comment.
I find that weird too. After we migrate the legacy charts and find ourselves in a more consolidated state, we should have another round to polish the form data to rename/remove some keys.
| rename_keys = { | ||
| "x_axis_label": "x_axis_title", | ||
| "x_axis_format": "x_axis_time_format", | ||
| "x_ticks_layout": "xAxisLabelRotation", |
There was a problem hiding this comment.
Why is there a mix of snake- and camel-case keys?
There was a problem hiding this comment.
Because the controls were defined using non-standard nomenclature 😢
| if self.data.get("contribution"): | ||
| self.data["contributionMode"] = "row" | ||
| if contribution := self.data.get("contribution"): | ||
| self.data["contributionMode"] = "row" if contribution else None |
There was a problem hiding this comment.
Given, per line #48, contribution is truthy this should reduce to:
if self.data.get("contribution"):
self.data["contributionMode"] = "row"There was a problem hiding this comment.
This code will be inherited from the base TimeseriesChart that was created in the Line migration. I will rebase this PR on top of that one.
| self.data["contributionMode"] = "row" if contribution else None | ||
|
|
||
| show_brush = self.data.get("show_brush") | ||
| self.data["zoomable"] = False if show_brush == "no" else True |
There was a problem hiding this comment.
| self.data["zoomable"] = False if show_brush == "no" else True | |
| self.data["zoomable"] = not self.data.get("show_brush") == "no" |
There was a problem hiding this comment.
This code will be inherited from the base TimeseriesChart that was created in the Line migration. I will rebase this PR on top of that one.
| self.data["x_axis_title"] = x_axis_label | ||
| self.data["x_axis_title_margin"] = 30 | ||
| if x_ticks_layout := self.data.get("x_ticks_layout"): | ||
| self.data["x_ticks_layout"] = 45 if x_ticks_layout == "45°" else 0 |
There was a problem hiding this comment.
Is 0 akin to None or undefined? If so there's probably no need to set it.
There was a problem hiding this comment.
No. It should be a number to match the type definition.
| self.data["opacity"] = 0.7 | ||
|
|
||
| if rolling_type := self.data.get("rolling_type"): | ||
| self.data["rolling_type"] = None if rolling_type == "None" else rolling_type |
There was a problem hiding this comment.
There was a problem hiding this comment.
This code will be inherited from the base TimeseriesChart that was created in the Line migration. I will rebase this PR on top of that one.
| x_axis_label = self.data.get("x_axis_label") | ||
| bottom_margin = self.data.get("bottom_margin") | ||
| if x_axis_label and (not bottom_margin or bottom_margin == "auto"): | ||
| self.data["bottom_margin"] = 30 |
There was a problem hiding this comment.
There was a problem hiding this comment.
This code will be inherited from the base TimeseriesChart that was created in the Line migration. I will rebase this PR on top of that one.
|
|
||
| x_axis_label = self.data.get("x_axis_label") | ||
| bottom_margin = self.data.get("bottom_margin") | ||
| if x_axis_label and (not bottom_margin or bottom_margin == "auto"): |
There was a problem hiding this comment.
There was a problem hiding this comment.
This code will be inherited from the base TimeseriesChart that was created in the Line migration. I will rebase this PR on top of that one.
michael-s-molina
removed
the
v3.0
|
Hi @michael-s-molina! When do we plan to merge it? |
4.0 given that the window for breaking changes is now closed. |
rusackas
added
the
v4.0
|
Closing this PR in favor of #25952 which makes it possible to migrate the chart without introducing a breaking change. |
SUMMARY
This PR migrates the Area chart to the ECharts version.
Note a previous migration existed however the legacy chart wasn't removed and thus new legacy Area charts could have been created.
AFTER SCREENSHOT
TESTING INSTRUCTIONS
1 - Make sure all Area (legacy) charts were converted to the ECharts version
2 - Make sure Area (legacy) is not available anymore in the viz picker
3 - Make sure that it's possible to revert the migration by executing superset db downgrade and reverting this PR
ADDITIONAL INFORMATION