Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: echarts gauge chart #993

Merged
merged 6 commits into from
Apr 7, 2021

Conversation

krsnik93
Copy link
Contributor

@krsnik93 krsnik93 commented Mar 5, 2021

🏆 Enhancements

Echarts gauge type chart in Superset.

I would appreciate feedback as the echarts chart API is really flexible, but when I look at other chart types already supported in Superset, they all expose a fairly simple API, so there are questions of just how to simplify.

For example I don't think the initial attempt would do well at imitating something like this: https://echarts.apache.org/examples/en/editor.html?c=gauge-ring.

A couple of screenshots of what's currently possible:
superset_gauge_1
superset_gauge_2

I am also a Typescript newbie so some of the typing might be weird.

@krsnik93 krsnik93 requested a review from a team as a code owner March 5, 2021 20:12
@vercel
Copy link

vercel bot commented Mar 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/14U7cgWR4J13dd2B2W8jqYY9zX5p
✅ Preview: https://superset-ui-git-fork-krsnik93-echarts-gauge-chart-superset.vercel.app

@villebro
Copy link
Contributor

villebro commented Mar 5, 2021

@krsnik93 this is AWESOME! ❤️ I'll post a review shortly - in the meantime feel free to annotate the code with comments if there are any areas you have questions about so we can address those properly.

@eugeniamz
Copy link

I can't wait to have this in master!! @krsnik93 Thanks!!

@rusackas
Copy link
Member

rusackas commented Mar 5, 2021

While perhaps not everyone loves gauge charts, I think this is a great thing to have in our collective arsenal. They are easily understandable in certain dashboards, where you want to see a benchmark or report-card KPI for some data point within known bounds and good/warning/bad thresholds. Examples:
• ISP speed tests compared to expected ranges (e.g. "am I actually GETTING gigabit?)
• A brewery's utilization of theoretical fermenter capacity, with R/A/G coloring of zones for loss/break-even/profit

Thanks for the work on this!

Small detail, but don't forget to add a thumbnail ;)

@eugeniamz
Copy link

@rusackas gouge is a key chart for KPI dashboards, having a visual image of how is the performance makes a big difference, big number says nothing about the performance. I am very excited about this chart and I can't wait to see in Master (I already said that 😃 )

@krsnik93 krsnik93 changed the title [WIP] feat: echarts gauge chart feat: echarts gauge chart Mar 8, 2021
@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #993 (07763d3) into master (54cee3a) will increase coverage by 0.02%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #993      +/-   ##
==========================================
+ Coverage   28.10%   28.13%   +0.02%     
==========================================
  Files         412      435      +23     
  Lines        8411     8822     +411     
  Branches     1193     1322     +129     
==========================================
+ Hits         2364     2482     +118     
- Misses       5894     6162     +268     
- Partials      153      178      +25     
Impacted Files Coverage Δ
...ns/plugin-chart-echarts/src/Gauge/EchartsGauge.tsx 0.00% <0.00%> (ø)
plugins/plugin-chart-echarts/src/Gauge/index.ts 0.00% <0.00%> (ø)
plugins/plugin-chart-echarts/src/utils/controls.ts 75.00% <60.00%> (-25.00%) ⬇️
...s/plugin-chart-echarts/src/Gauge/transformProps.ts 85.10% <85.10%> (ø)
...ugins/plugin-chart-echarts/src/Gauge/buildQuery.ts 100.00% <100.00%> (ø)
...lugins/plugin-chart-echarts/src/Gauge/constants.ts 100.00% <100.00%> (ø)
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx 100.00% <100.00%> (ø)
plugins/plugin-chart-echarts/src/Gauge/types.ts 100.00% <100.00%> (ø)
plugins/plugin-chart-table/src/controlPanel.tsx 0.00% <0.00%> (-34.49%) ⬇️
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx 66.66% <0.00%> (-33.34%) ⬇️
... and 72 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54cee3a...07763d3. Read the comment docs.

@junlincc
Copy link
Contributor

junlincc commented Mar 9, 2021

Thank you so much @krsnik93 for your contribution! this is really awesome. ♥️

I agreed with @rusackas; Gauge is pretty common in modern BI products. It's not only great at capturing attention from the audience by providing a single quantitative value, but also indicating where the value stands within a standard range... anyway, it's an ideal chart for business users/executives, whom we haven't been focusing on.

I would be curious to know where the contentious points are, if there are any? Hopefully, we can address the concerns before bringing the chart to the codebase? @williaster 🙏

@junlincc
Copy link
Contributor

junlincc commented Mar 9, 2021

@mayurnewase would you like to review this PR? 🙏

Copy link
Contributor

@mayurnewase mayurnewase left a comment

Choose a reason for hiding this comment

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

Added first pass comments.
Having problems with reading values using the default setting, I might be doing something wrong.
Screenshot_2021-03-10  DEV  Explore - energy_usage(2)
Edit: This happens when multiple rows are returned from backend.

plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Outdated Show resolved Hide resolved
valueAnimation,
valueFormatter,
}: EchartsGaugeFormData = { ...DEFAULT_GAUGE_FORM_DATA, ...formData };
const data = queriesData[0].data as TimeseriesDataRecord[];
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check if it's null then use empty array?

Copy link
Contributor

@mayurnewase mayurnewase Mar 10, 2021

Choose a reason for hiding this comment

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

Any specific reason to prefer timeSeriesRecord over dataRecord?
Are we using the timestamp field ?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should ensure queriesData[0] not null.

plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Outdated Show resolved Hide resolved

if (!intervalBoundsAndColors.length) {
progress = {
show: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be moved to a separate constants file and type it as well?

plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Outdated Show resolved Hide resolved
fontSize: 1.5 * fontSize,
};
itemStyle = {};
} else {
Copy link
Contributor

@mayurnewase mayurnewase Mar 10, 2021

Choose a reason for hiding this comment

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

If the only difference in if,else is line and item style, should we just create a function to get it from interval bound setting?

plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx Outdated Show resolved Hide resolved
valueAnimation,
valueFormatter,
}: EchartsGaugeFormData = { ...DEFAULT_GAUGE_FORM_DATA, ...formData };
const data = queriesData[0].data as TimeseriesDataRecord[];
Copy link
Contributor

Choose a reason for hiding this comment

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

we should ensure queriesData[0] not null.

plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/transformProps.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Gauge/types.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM - just a minor comment about the color scheme, but unless there are other comments/objections I'll merge this in 24 h

Comment on lines +258 to +272
'Comma-separated interval bounds, e.g. 2,4,5 for intervals 0-2, 2-4 and 4-5. Last number should match the value provided for MAX.',
),
renderTrigger: true,
default: DEFAULT_FORM_DATA.intervals,
},
},
],
[
{
name: 'interval_color_indices',
config: {
type: 'TextControl',
label: t('Interval colors'),
description: t(
'Comma-separated color picks for the intervals, e.g. 1,2,4. Integers denote colors from the chosen color scheme and are 1-indexed. Length must be matching that of interval bounds.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could take the modulo on the the length of the color scheme when using it, thereby looping and avoiding errors.

@villebro villebro merged commit d2873ee into apache-superset:master Apr 7, 2021
@villebro
Copy link
Contributor

villebro commented Apr 7, 2021

Merged - thanks so much for your patience with the review process @krsnik93 ! We need to bump the core package first, but once that's done we can proceed to open up a PR on the main repo to get this in :) I'll keep you posted when we can do that (you should do the honors!)

@junlincc
Copy link
Contributor

junlincc commented Apr 7, 2021

Let's bump one chart at a time.

@villebro
Copy link
Contributor

FYI @krsnik93 - superset-ui has now been bumped on the main repo (apache/superset#14045), so feel free to open a PR adding this plugin to Superset! 🙂 Let me know if you need help.

import { DEFAULT_FORM_DATA } from './types';

const config: ControlPanelConfig = {
controlPanelSections: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add Time section? I think it's very useful.
Like:

NejcZdovc pushed a commit to blotoutio/superset-ui that referenced this pull request Apr 20, 2021
* feat: echarts gauge chart

* remove unused legend imports

* move font size multipliers to constants

* add tests

* rename roundcap

* add modulo on color picking to wrap around the color scheme

Co-authored-by: Ivan Krsnik <ivan.krsnik@unipart.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.