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

feat(plugin-chart-echarts): [wip] gauge chart enhancements and fixes #1070

Closed

Conversation

krsnik93
Copy link
Contributor

🏆 Enhancements

This PR addresses feature requests and bugs raised here: apache/superset#14209

@krsnik93 krsnik93 requested a review from a team as a code owner April 23, 2021 12:23
@vercel
Copy link

vercel bot commented Apr 23, 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/EGkJ6kzbWtJZCpE9UyXfcwZPzH9t
✅ Preview: https://superset-ui-git-fork-krsnik93-gauge-enhancements-and-8fd53b.vercel.app

@codecov
Copy link

codecov bot commented Apr 23, 2021

Codecov Report

Merging #1070 (65cebcc) into master (e675d99) will increase coverage by 0.00%.
The diff coverage is 60.00%.

❗ Current head 65cebcc differs from pull request most recent head 40284a2. Consider uploading reports for the commit 40284a2 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1070   +/-   ##
=======================================
  Coverage   28.74%   28.75%           
=======================================
  Files         455      455           
  Lines        9135     9144    +9     
  Branches     1436     1438    +2     
=======================================
+ Hits         2626     2629    +3     
- Misses       6306     6310    +4     
- Partials      203      205    +2     
Impacted Files Coverage Δ
...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx 100.00% <ø> (ø)
plugins/plugin-chart-echarts/src/Gauge/types.ts 100.00% <ø> (ø)
...s/plugin-chart-echarts/src/Gauge/transformProps.ts 76.66% <60.00%> (-7.65%) ⬇️

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 e675d99...40284a2. Read the comment docs.

@junlincc
Copy link
Contributor

junlincc commented May 5, 2021

@krsnik93 many thanks for the PR! is it ready to turn draft to open?

@krsnik93 krsnik93 marked this pull request as ready for review May 5, 2021 08:07
@@ -127,7 +127,15 @@ export default function transformProps(chartProps: ChartProps) {
fontSize: FONT_SIZE_MULTIPLIERS.detailFontSize * fontSize,
},
}));

const min = minVal || calculateMin(transformedData);
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 use ?? to replace || ?I'm not sure if it needs to be recalculated only when minVal is null instead of 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not aware of the ?? operator. Exactly what is needed here. Thanks a lot!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's ok :). ?? is called Nullish Coalescing which just for the case of null and undefined values. Here, if we use the || operator, it will also be calculated when the value of minVal is 0, which is actually unnecessary.So it may be better to use nullish coalescing.

@@ -70,6 +71,12 @@ const setIntervalBoundsAndColors = (
const calculateAxisLineWidth = (data: DataRecord[], fontSize: number, overlap: boolean): number =>
overlap ? fontSize : data.length * fontSize;

const calculateMin = (data: GaugeDataItemOption[]) =>
2 * Math.min(...data.map(d => d.value as number).concat([0]));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why here must be 2 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not have to be, I was looking at how other BI tools do this and came up with 2. We could have 1.5, 1.2 or similar. The point is to ensure that the max value fits into the chart, but that there is a bit of additional "space" left on the gauge.

@junlincc
Copy link
Contributor

@krsnik93 thanks for addressing the comments. can we have one line description on each commit please 😇

Add a hover tooltip per pointer on gauge chart showing metric names and values
root added 2 commits May 12, 2021 09:31
…rt axis

Dynamically determine min and max values on the axis from query result unless params explicitly passed through form data
Fix ts errors
@villebro
Copy link
Contributor

The codebase on this repo has been moved to the main Apache Superset repo, and consequently the repo is in the process of being archived. See the Superset Improvement Proposal for details: apache/superset#13013 . While all currently open issues and PRs will be closed, we encourage you to reopen this PR on the main repo, which should be as simple as moving over any code changes as follows:

  • core packages: packages/** (this repo) -> superset-frontend/packages/** (main repo)
  • plugins: plugins/** (this repo) -> superset-frontend/plugins/** (main repo)

If you need help with the migration, please post a message on the SIP or reach out on the community Slack.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants