superset icon indicating copy to clipboard operation
superset copied to clipboard

feat: Utility function to render chart tooltips

Open michael-s-molina opened this issue 1 year ago • 4 comments

SUMMARY

This PR adds an utility function to render chart tooltips with the following objectives:

  • Add the ability to display total and percentage values for our tooltips. This improvement will also allow us to migrate legacy timeseries charts, which currently display that information, to ECharts.
  • Standardize the way we display tooltips. Currently, every plugin displays tooltips using different information and designs, which decreases the overall user experience. By creating a reusable function, we can ensure consistency across plugins.
  • Reduce code duplication.

This PR will only format ECharts tooltips to limit the scope of work but other plugins can also benefit from the utility function.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshot 2024-04-09 at 10 40 12 Screenshot 2024-04-09 at 09 27 11 Screenshot 2024-04-09 at 10 38 21 Screenshot 2024-04-16 at 15 17 05 Screenshot 2024-04-09 at 10 42 09 Screenshot 2024-04-09 at 09 37 36 Screenshot 2024-04-10 at 10 19 15 Screenshot 2024-04-10 at 11 23 46 Screenshot 2024-04-16 at 10 55 53 Screenshot 2024-04-16 at 11 37 02 Screenshot 2024-04-16 at 15 02 27 Screenshot 2024-04-16 at 15 08 56 Screenshot 2024-04-16 at 15 23 41 Screenshot 2024-04-16 at 15 34 21 Screenshot 2024-04-18 at 13 37 20 Screenshot 2024-04-18 at 13 41 16 Screenshot 2024-04-18 at 14 12 42 Screenshot 2024-04-18 at 14 48 27 Screenshot 2024-04-19 at 09 12 08 Screenshot 2024-04-19 at 09 24 32 Screenshot 2024-04-19 at 11 35 04 Screenshot 2024-04-19 at 11 50 08 Screenshot 2024-04-19 at 13 22 01 Screenshot 2024-04-19 at 13 25 55 Screenshot 2024-04-19 at 13 30 42 Screenshot 2024-04-19 at 13 43 55 Screenshot 2024-04-19 at 13 49 11 Screenshot 2024-04-19 at 13 56 33

TESTING INSTRUCTIONS

Test consists in checking how the tooltip is displayed for all ECharts plugins and their different control combinations.

ADDITIONAL INFORMATION

  • [ ] Has associated issue:
  • [ ] Required feature flags:
  • [ ] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] Migration is atomic, supports rollback & is backwards-compatible
    • [ ] Confirm DB migration upgrade and downgrade tested
    • [ ] Runtime estimates and downtime expectations provided
  • [ ] Introduces new feature or API
  • [ ] Removes existing feature or API

michael-s-molina avatar Apr 09 '24 14:04 michael-s-molina

One question I have isn't about the PR itself (which is lookin' good!) but about the new component's potential future, and thus its location in the codebase.

Since the TooltipRenderer component doesn't seem to rely on anything ECharts related, do you think it would make sense to move it out of the ECharts plugin and into Superset-UI/core or similar? It seems like we could use it on other plugins (ECharts or not) to standardize the UI even further across Superset. Having that might also pave the way to add a customization controls to it down the road, enabling templating (handlebars/jinja/whatever) and replacing the JS Tooltips used by DeckGL.

rusackas avatar Apr 09 '24 17:04 rusackas

@justinpark @villebro @rusackas Thank you for the comments. I'm still working on the component's API and implementation as I continue to iterate on each plugin. I'm still not sure what will be the final API and implementation because each plugin has a unique set of requirements. For that reason, I'll first make sure all ECharts plugins are using the renderer, to have a clear view of all requirements, and then work on optimizations. Feel free to review this PR while it evolves or wait for when it leaves the draft state.

michael-s-molina avatar Apr 10 '24 12:04 michael-s-molina

Wow this is a nice improvement! 🤩 Thank you for including all of the before/after screenshots. This will implement the following feature requests:

  1. #18462
  2. #28216

sfirke avatar Apr 30 '24 14:04 sfirke

Thanks for linking the resources @sfirke!

michael-s-molina avatar Apr 30 '24 14:04 michael-s-molina

Really nice feature indeed ! Will this feature be also available for the other types of charts (Country Map, World Map, Bar chart...) any time soon ? Thanks

igamat avatar Jun 07 '24 09:06 igamat

Really nice feature indeed ! Will this feature be also available for the other types of charts (Country Map, World Map, Bar chart...) any time soon ? Thanks

I think we all WANT it to get there, but it's probably not a high priority at the moment. If you have the means to contribute that as a PR, we'd be super excited to review it!

rusackas avatar Jun 07 '24 19:06 rusackas