evidence-dev / evidence

Business intelligence as code: build fast, interactive data visualizations in pure SQL and markdown
https://evidence.dev
MIT License
3.44k stars 167 forks source link

Add value labels to charts #1255

Closed hughess closed 7 months ago

hughess commented 7 months ago

Chart Value Labels

This PR adds value labels to bar, line, and area charts.

CleanShot 2023-10-12 at 09 44 36@2x

Labels are turned off by default, but can be added by setting labels=true

This also includes a variety of props to format labels:

By default, label overlap is avoided, but there is an option to show everything:

In stacked bar charts, we’ve added a stack total label which appears above the bars. This can be turned off with:

Defaults

Limitations

On stacked bar charts, the total label does not recalculate when hiding series by clicking on the legend. For now, I have turned off that type of interaction when a stacked total is present - otherwise, you could deselect a series and have the wrong total sitting on top of the bars.

This PR doesn’t include value labels for scatter plots, bubble charts, or histograms.

Scatter/bubble labels are up next, but involve some challenges because the approach will include the ability to pass a specific column to the label.

One issue that is unsolved here is the legend hover state. When you hover over a series in a legend, all labels will appear on the hovered series, including any overlap or conflicts. I've opened an issue with ECharts about this: https://github.com/apache/echarts/issues/19192

Questions

Notes on changes to charting library

Checklist

changeset-bot[bot] commented 7 months ago

🦋 Changeset detected

Latest commit: a87a9cb0b17954db5358c47764bae945012228ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages | Name | Type | | --------------------------------- | ----- | | @evidence-dev/component-utilities | Minor | | @evidence-dev/core-components | Minor | | @evidence-dev/components | Minor | | @evidence-dev/evidence | Major | | evidence-test-environment | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 7 months ago

Deploy Preview for evidence-development-workspace ready!

Name Link
Latest commit a87a9cb0b17954db5358c47764bae945012228ed
Latest deploy log https://app.netlify.com/sites/evidence-development-workspace/deploys/6529d7b1e77acd0008a2de91
Deploy Preview https://deploy-preview-1255--evidence-development-workspace.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
evidence-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 13, 2023 11:50pm
hughess commented 7 months ago

Text Flickering Issue

Noticed this on Safari and checked out all the browsers I have. This reminds me of a previous issue I had with outlined text, where having a thin outline around characters caused some browsers to not render it correctly:

CleanShot 2023-10-12 at 10 12 42

Mac Desktop

Windows Desktop

Mobile

Printing/PDF seems okay on all browsers as far as I can tell, it's just the hover interaction that seems to cause this

https://github.com/apache/echarts/issues/19199

hughess commented 7 months ago

Update on flickering issue

This is caused by the textBorder option in ECharts. We could turn it off, but it's very helpful in managing the contrast in labels compared to their backgrounds.

This issue only happens with the SVG renderer in ECharts. Switching to Canvas renderer solves the problem on desktop browsers. Still need to confirm for mobile browsers.

Should we change to the canvas renderer?

@ItsMeBrianD @csjh @mcrascal @archiewood

Initially I picked the SVG renderer because it maintains 100% crisp visuals when you zoom in vs. blurry with Canvas.

Here's ECharts' description of choosing a renderer:

Generally, Canvas is more suitable for charts with a large number of elements (heat map, large-scale line or scatter plot in geo or parallel coordinates, etc.), and with visual effect. However, SVG has an important advantage: It has less memory usage (which is important for mobile devices) and won't be blurry when zooming in.

I'm not sure how to check the performance/memory impacts of making the change.

Does anyone have concerns with changing to the canvas renderer?

csjh commented 7 months ago

I think some features are locked to certain renderers, though I'm looking through the docs now and the only one they mention is renderToSVGString (which we don't use)

hughess commented 7 months ago

UI test fails after changing to canvas because the test looks for a <g> element. Have tested the functionality and it's working

hughess commented 7 months ago

Realized this is going to need to work with the secondary y-axis as well.

That may mean changing the prop names to align to an axis. i.e.:

This may be confusing when compared to yAxisLabels and y2AxisLabels, so maybe adding "value" to these props is a good idea despite the longer names:

archiewood commented 7 months ago

do you need to be able to control the two axes labels independently?

If you had two series on the same axis, you wouldn't be able to to that

Or could they both be controlled by one set of props?

labels
labelFmt
labelPosition
labelColor
labelSize
showAllLabels
stackTotalLabel

The only one I can see a reason to have independent control over is the fmt

hughess commented 7 months ago

I think fmt would be the most common, but I could also see position - e.g., you have a bar and want the labels to be positioned "inside" vs. "above" for the line on the other axis

archiewood commented 7 months ago

postion

you can't control position in a multi series line chart independently, so I would be happy to not support that.

fmt

with two axes there is a decent chance the formats would be different.

I'd probably have:

labelFmt - adjusts both yLabelFmt - adjusts first axis, overrideing labelFmt if specified y2LabelFmt - adjusts second axis, overrideing labelFmt if specified

hughess commented 7 months ago

good point - i had already forgotten that y2 only works with lines now. Agree with those