refactor: Removes 5.0 approved legacy charts #31582
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
|
michael-s-molina
force-pushed
the
remove-approved-legacy-charts
branch
2 times, most recently
from
efb5f49 to
80d3f75
Compare
| @@ -255,13 +243,16 @@ function saveExploreColorScheme( | |||
| cy.wait('@chartUpdate'); | |||
| } | |||
|
|
|||
| // FIXME: Skipping some tests as ECharts are rendered using Canvas and we cannot inspect the elements | |||
There was a problem hiding this comment.
@geido The color scheme tests were using a legacy chart to check the applied colors. Given that ECharts are rendered using Canvas, we'll need a solution to be able to inspect the elements. I'm skipping these tests for now and adding a FIXME comment.
There was a problem hiding this comment.
It feels like we could optimize this test, too, and use example dashboards to do fewer round trips between Dashboard and Chart Editor (though, maybe optimization is secondary, and round trip testing is a worthy effort too ¯_(ツ)_/¯)
There was a problem hiding this comment.
Yeah, that's a good suggestion. One option I can think of to overcome the ECharts canvas problem and be able to identify color changes would be to use the Word Cloud plugin which is pure HTML. I believe the solution for this problem should be resolved in a follow-up PR. I already sent a message to @geido about this.
michael-s-molina
force-pushed
the
remove-approved-legacy-charts
branch
2 times, most recently
from
159e19c to
9f29d94
Compare
michael-s-molina
requested review from
sadpandajoe,
geido,
eschutho,
rusackas,
betodealmeida and
mistercrunch
as code owners
dosubot
bot
added
risk:breaking-change
michael-s-molina
force-pushed
the
remove-approved-legacy-charts
branch
from
424459a to
5cb926f
Compare
michael-s-molina
force-pushed
the
remove-approved-legacy-charts
branch
from
5cb926f to
a68cd5b
Compare
|
@betodealmeida @eschutho @mistercrunch Can you stamp this PR as code owners? |
|
WOOHOO!!!!! |
|
cuando lanzan superset 5 ?? |
|
@ryepezb1 You check the schedule for major releases in our Release Process. |
SUMMARY
As part of the 5.0 breaking window, this PR executes the following proposals:
I decided to do it all in a single PR to have a single migration for the charts. There's also many code blocks that were removed that touches multiple visualization types at the same time and it would be more complex to remove it in parts. Another important factor was the ability to execute the tests once, given that we need to check if the example charts are loaded correctly. In summary, the benefits outweigh the review complexity.
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
Description by Korbit AI
What change is being made?
Remove legacy charts (Area, Bar, Event Flow, Heatmap, Histogram, Line, Sankey, and Sankey Loop) and migrate them to their ECharts counterparts, while updating relevant tests and dependencies.
Why are these changes being made?
The legacy charts were deprecated and have been fully migrated to ECharts versions to take advantage of modern features and better maintainability, except for Event Flow and Sankey Loop charts, which have been removed due to not being actively maintained or widely used. This refactor reduces codebase clutter and ensures developers work with up-to-date and supported charting tools.