Refactor and fix chart show value plugin#8916
Open
hokolomopo wants to merge 13 commits into
Open
Conversation
Collaborator
|
|
A lot of the behaviour of our ChartJS' show value plugin were not tested. This commit adds a bunch of tests for it. Note that some test screenshots show clearly wrong results. These will be fixed in later commits, this commit only introduce tests to have a baseline to test against. Task: 6123207
Our chartJS plugin `chartShowValuesPlugin` is a bit of a mess, with a bunch of duplicated code, making it very hard to read and maintain. As a proof the number of bugs to draw the values fixed in the following commits, and visible in the screenshot tests. This commit aims to refactor the plugin. The core logic is now in the `drawValues` helper, which take callbacks for the chart-specific logic. The goal of this commit is only to rewrite the code without fixing or modifying any functional behaviour, these will be fixed in follow-up commits. Task: 6123207
This commits splits the chimeric monstrosity `drawLineOrBarOrRadarChartValues` in separate function for each chart type. Task: 6123207
On bar charts, use the actual measured text height to position the values instead of the font size. Task: 6123207
The radar charts didn't have a color set for the background of their points. By default ChartJS use the same color as the dataset background, which is transparent for filled radar charts. This commit adds an explicit color for the points, so neither the points nor the values are half transparent for filled radar charts. Task: 6123207
With this commit, the values drawn inside a bar/pie chart element will now be a text colored with he chart background color, with a halo of the color of the dataset, instead of being chart-dependant. Task: 6123207
When hovering a legend item, all the other dataset become transparent. This make the shown values become transparent and a bit ugly. But if a dataset is made transparent and half-hidden, the shown values don't make much sense and can be skipped. Task: 6123207
- The bubble chart show value didn't match the color of the other types oft charts - If the bubble didn't have a set size, the show value text would go over the bubble and hide the point Task: 6123207
For very small funnel bars, the text wasn't centered. It's because we re-used the code of horizontal bar charts, which offset the values too close to the axis origin. Something we don't want for funnel charts. Task: 6123207
When drawing negative values next to a point element in a chart (line, scatter, radar and bubble chart), we could draw the value above the point for positive values, and below for negative values. The distinction was arbitrary and didn't always make sense, remove it. Task: 6123207
Calendar charts were doing some try-hard hacky stuff, computing a false background color of the calendar element by recomputing the color scale. This can be simplified by simply fetching the color of the drawn chart element. Task: 6123207
The combo chart show value did every computation assuming the dataset was a bar chart dataset, gicing wrong results for the line datasets. Task: 6123207
In the chart show value plugin, we use the text positions as key of an object. This can cause some issues with rounding errors. Task: 6123207
hokolomopo
force-pushed
the
master-chart-dhow-values-ref-adrm
branch
from
c1bc801 to
3c146af
Compare
hokolomopo
commented
Jun 15, 2026
Comment on lines
64
to
+71
| switch (options.type) { | ||
| case "pie": | ||
| drawPieChartValues(chart, options, ctx); | ||
| drawPieValues(chart, options); | ||
| break; | ||
| case "line": | ||
| case "scatter": | ||
| drawLineValues(chart, options); | ||
| break; |
Contributor
Author
There was a problem hiding this comment.
There's a world where instead of this switch, all the chart specific logic is inside chartjs_show_values.ts and the callbacks are created when creating the runtime rather than here in the chartJs plugin.
It'd be probably a bit better ? I'll wait for another opinion before moving everything.
Comment on lines
+18
to
+20
| interface CallbackArgs { | ||
| dataset: any; | ||
| chartElement: any; |
Contributor
Author
There was a problem hiding this comment.
typing this probably isn't worth, we keep using "private" properties and having to ts-ignore
hokolomopo
commented
Jun 16, 2026
| @@ -1,10 +1,30 @@ | |||
| import type { ChartMeta, ChartType, Plugin } from "chart.js"; | |||
Contributor
Author
There was a problem hiding this comment.
Also some of the fix commits maybe should be backported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
description of this task, what is implemented and why it is implemented that way.
Task: 6123207
review checklist