-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: Small tweaks for Line and Area chart migrations (ECharts) #28334
fix: Small tweaks for Line and Area chart migrations (ECharts) #28334
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #28334 +/- ##
==========================================
+ Coverage 60.49% 70.04% +9.54%
==========================================
Files 1931 1933 +2
Lines 76241 76500 +259
Branches 8566 8567 +1
==========================================
+ Hits 46122 53583 +7461
+ Misses 28015 20813 -7202
Partials 2104 2104
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -128,6 +128,10 @@ def _pre_action(self) -> None: | |||
): | |||
self.data["bottom_margin"] = 30 | |||
|
|||
left_margin = self.data.get("left_margin") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just ensuring consistency with other EChart implementations?
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
…transformProps.ts
Co-authored-by: John Bodley <[email protected]> (cherry picked from commit b4ab36a)
…e#28334) Co-authored-by: John Bodley <[email protected]>
…e#28334) Co-authored-by: John Bodley <[email protected]>
…e#28334) Co-authored-by: John Bodley <[email protected]>
…e#28334) Co-authored-by: John Bodley <[email protected]> (cherry picked from commit a8ba9ab)
…e#28334) Co-authored-by: John Bodley <[email protected]>
SUMMARY
Fixes some issues found while testing Line and Area chart migrations (ECharts) using Airbnb's production data:
groupby
is an array. Even though the Typescript definition does not allownull
values, legacy charts might have them. Check fix: Ensures form data required properties exist at runtime (echarts-timeseries) #25419 for more context about this problem.TESTING INSTRUCTIONS
CI is sufficient as I'm already testing the migrations with a lot of data.
ADDITIONAL INFORMATION