Skip to content
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

feat(echarts-pie): add string template support for labels #28774

Merged
merged 9 commits into from
Jun 12, 2024

Conversation

hexcafe
Copy link
Contributor

@hexcafe hexcafe commented May 30, 2024

SUMMARY

Currently, we support several static label types:

  • key
  • value
  • percent
  • key_value
  • key_percent
  • key_value_percent
  • value_percent

Providing a dynamic way for users to customize labels could be a great and innovative feature, making our pie charts more useful.

We have introduced a new label type called template to support this feature. Users can customize the label by writing a string template. The string template supports the following variables:

  • {name} the name of a data item
  • {value} the value of a data item, formatted with number format option
  • {percent} the percentage of a data item, formatted with number formatter

Additionally, for ECharts compatibility, the following variables are supported:

  • {a} series name.
  • {b} the name of a data item, unformatted
  • {c} the value of a data item, unformatted
  • {d} percentage, unformatted

\n is supported to represent a new line.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Formatted variables:
image

ECharts compatible variables:
image

TESTING INSTRUCTIONS

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

@dosubot dosubot bot added the viz:charts:pie Related to the Pie chart label May 30, 2024
@rusackas
Copy link
Member

rusackas commented Jun 4, 2024

/testenv up

Copy link
Contributor

github-actions bot commented Jun 4, 2024

@rusackas Ephemeral environment spinning up at http://34.223.245.15:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

rusackas commented Jun 4, 2024

This is pretty cool!!! Think it's worth adding some tests to make sure things display as expected? We don't need to test that ECharts works as expected, but maybe just make sure the transformProps file works (and continues to work) as expected.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but leaving it open awaiting feedback about the testing comment.

@hexcafe
Copy link
Contributor Author

hexcafe commented Jun 5, 2024

This is pretty cool!!! Think it's worth adding some tests to make sure things display as expected? We don't need to test that ECharts works as expected, but maybe just make sure the transformProps file works (and continues to work) as expected.

Thanks @rusackas , I'll add some tests later

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 5, 2024
@villebro
Copy link
Member

villebro commented Jun 5, 2024

I also think this is a great direction, and one we should consider for all charts. As this can be difficult to change after merging, I would almost doing a quick SIP round to make sure we have a solution that is generic enough to work for all charts. CC @michael-s-molina

@michael-s-molina
Copy link
Member

michael-s-molina commented Jun 5, 2024

While working on SIP-50, I noticed many inconsistencies between ECharts plugins. They range from missing control types to control types that treat the same information differently. I believe the main cause for the problem is because we treat each plugin individually when adding new features. This leads to duplicated code and poor UX. My plan is to review all Explore controls after we complete the ECharts migrations and make sure they are consistent between plugins (same code, same behavior).

I would almost doing a quick SIP round to make sure we have a solution that is generic enough to work for all charts.

This would certainly help given that the control and code would be the same for all plugins.

@michael-s-molina
Copy link
Member

Another thing is that we would need a clear description of how the template works and which variables are available. Maybe in the form of a tooltip.

@rusackas
Copy link
Member

rusackas commented Jun 5, 2024

There are already instructions on a tooltip in this PR... you can see it in the ephemeral env if you want to try it out :)

@rusackas
Copy link
Member

rusackas commented Jun 5, 2024

image

I guess we could elaborate more here (like what a/b/c/d are) but it's there! :D

@michael-s-molina
Copy link
Member

@rusackas Thank you for the info.

I guess we could elaborate more here (like what a/b/c/d are) but it's there! :D

That would be helpful indeed.

@hexcafe
Copy link
Contributor Author

hexcafe commented Jun 5, 2024

@rusackas @michael-s-molina

I guess we could elaborate more here (like what a/b/c/d are) but it's there! :D

Does this look good?
image

@rusackas
Copy link
Member

rusackas commented Jun 5, 2024

@rusackas @michael-s-molina

I guess we could elaborate more here (like what a/b/c/d are) but it's there! :D

Does this look good? image

Why yes, that does help! 😍

@villebro
Copy link
Member

villebro commented Jun 6, 2024

I'll drop a related thought I've been playing around with: In Tableau it's possible to add the equivalent of metrics as tooltip fields. This is really useful, as you may want to have the main metric in the chart, but then display other related metrics for the same series. Also we should expose all existing formatters etc to make it possible to customize the final format. I feel this templating feature should support this. So if we were to make it possible to add additional metrics that don't show up on the chart, but would then be able to reference those on the tooltip, I think we'd close a big gap to Tableau. The flow could be something like this:

  1. Add an additional metric, say My Other Metric. Could be a new checkbox "Omit metric from chart" or a new control "Additional metrics". (neither is ideal, but given the current control panel, I think we need to make some compromise)
  2. Add a reference to the new metric in the tooltip. Something like this: {d3Formatter('My Other Metric', '.1s')} This would then render the new metric with the .1s d3 formatter.

The exact syntax and control solution for adding the extra metrics would need to be ironed out first, of course, and I propose doing this in a dedicated SIP for this.

@rusackas
Copy link
Member

rusackas commented Jun 7, 2024

Sounds like there might be more to enhance, but does anyone have any objection to merging?

mailchimp-button

@rusackas rusackas merged commit a067ffb into apache:master Jun 12, 2024
33 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas
Copy link
Member

Ok, so this is cool, it's useful, and it seems portable. Worth merging in my book!

I completely agree with the other points on the thread:
• This control should exist for as many ECharts plugins as possible
• We should make these plugins as consistent as possible. Maybe even make an "ECharts Options" section of controls that we can use between them in a more DRY way.
• A SIP around this sure couldn't hurt... it doesn't need to be a big one, but it's nice to have consensus. Even a "lazy consensus" thread might be sufficient for the scope above. If we want to add additional metrics into the templating like @villebro suggests, then this certainly warrants a full SIP. Happy to help if you'd like, @hexcafe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugins size/L viz:charts:pie Related to the Pie chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants