feat: Adds Line chart migration logic #23973
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michael-s-molina
added
risk:breaking-change
michael-s-molina
force-pushed
the
migrate-line
branch
from
3e8840d
to
bcfb878
Compare
michael-s-molina
force-pushed
the
migrate-line
branch
from
bcfb878
to
7791b92
Compare
john-bodley
reviewed
May 22, 2023
john-bodley
reviewed
May 22, 2023
john-bodley
reviewed
May 22, 2023
michael-s-molina
force-pushed
the
migrate-line
branch
from
7791b92
to
3480a33
Compare
michael-s-molina
removed
the
v3.0
9 tasks
3 tasks
michael-s-molina
force-pushed
the
migrate-line
branch
from
3480a33
to
808c8ac
Compare
michael-s-molina
removed
risk:breaking-change
michael-s-molina
changed the title
michael-s-molina
force-pushed
the
migrate-line
branch
from
1df9eb4
to
0200172
Compare
9 tasks
john-bodley
reviewed
Sep 25, 2023
michael-s-molina
force-pushed
the
migrate-line
branch
from
e39fa2c
to
f09881a
Compare
john-bodley
reviewed
Oct 31, 2023
superset/cli/viz_migrations.py
Outdated
| @@ -29,6 +29,7 @@ class VizType(str, Enum): | |||
| AREA = "area" | |||
| PIVOT_TABLE = "pivot_table" | |||
| SUNBURST = "sunburst" | |||
| LINE = "line" | |||
| has_x_axis_control = True | ||
|
|
||
| def _pre_action(self) -> None: | ||
| self.data["contributionMode"] = "row" if self.data.get("contribution") else None |
There was a problem hiding this comment.
Is this (and other attributes) required? Generally if not I think it's cleaner/safer to leave these undefined and fallback to the default.
There was a problem hiding this comment.
These rules were added to keep visual consistency between the previous and new versions.
|
|
||
| def _pre_action(self) -> None: | ||
| self.data["contributionMode"] = "row" if self.data.get("contribution") else None | ||
| self.data["zoomable"] = False if self.data.get("show_brush") == "no" else True |
There was a problem hiding this comment.
Suggested change
| self.data["zoomable"] = False if self.data.get("show_brush") == "no" else True | |
| self.data["zoomable"] = self.data.get("show_brush") != "no" |
| ): | ||
| self.data["bottom_margin"] = 30 | ||
|
|
||
| if (rolling_type := self.data.get("rolling_type")) and rolling_type != "None": |
There was a problem hiding this comment.
Any reason rolling_type is "None" as opposed to None?
There was a problem hiding this comment.
Many of these safe checks were added as a result of trying to run this migration using our production database which contained really old charts and inconsistent values.
|
|
||
| if time_compare := self.data.get("time_compare"): | ||
| self.data["time_compare"] = [ | ||
| value + " ago" for value in as_list(time_compare) if value |
There was a problem hiding this comment.
Are there causes when value is falsy?
There was a problem hiding this comment.
Same as above.
john-bodley
approved these changes
Nov 9, 2023
josedev-union
pushed a commit
to Ortege-xyz/studio
that referenced
this pull request
Jan 22, 2024
cccs-rc
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Mar 6, 2024
mistercrunch
added
🏷️ bot
sfirke
pushed a commit
to sfirke/superset
that referenced
this pull request
Mar 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
This PR adds the Line chart migration logic (NVD3 ➡️ ECharts). Users can execute this migration using the migrate_viz CLI command and disable the legacy version with the VIZ_TYPE_DENYLIST configuration.
@sadpandajoe @jinghua-qa
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
1 - Upgrade a Line chart using the CLI command
2 - Check the new chart
3 - Downgrade a Line chart using the CLI command
4 - Check the legacy chart
ADDITIONAL INFORMATION