Conversation
raf18seb
left a comment
raf18seb
left a comment
There was a problem hiding this comment.
Thanks for the PR! The solution is good, but it's narrowing the dataLabels TS type and might not work in all cases, so we need to discuss before merging. Please see my comments below.
Also, adding a regression test might be a good idea (at least something to consider too).
| minSize?: number; | ||
| maxSize?: number; | ||
| componentColors?: ComponentColors; | ||
| dataLabels?: Highcharts.DataLabelsOptions; |
There was a problem hiding this comment.
Are you sure you want to narrow down this type? What if the dataLabels are defined as an array?
If you're sure that we should resign from dataLabels array config, then at least please add an explanation comment.
There was a problem hiding this comment.
As SeriesOptions type doesn’t accept dataLablels, I added Highcharts.DataLabelsOptions[] to the type.
raf18seb
left a comment
raf18seb
left a comment
There was a problem hiding this comment.
Hi @annavvis, please double-check your solution because it's not solving the issue (the error still occurs). If I had to guess, I'd say you didn't use the simplified demo.
Remember that dataLabels can be enabled for a single point, not just at the series level.
Also, please see the comments below 👇
| // Data labels can be enabled either as a single object or as an array | ||
| // of objects (#5) | ||
| const dataLabels = this.options.dataLabels; | ||
| const dataLabelsEnabled = Array.isArray(dataLabels) |
There was a problem hiding this comment.
Consider using H.splat() to be consistent with Highcharts patterns.
| it('returns true when dataLabels is undefined', () => { | ||
| expect(labelsEnabled(undefined)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Why does labelsEnabled return true when dataLabels are undefined? Shouldn't it be the opposite?
| function labelsEnabled(dl: DL | DL[] | undefined): boolean { | ||
| return Array.isArray(dl) | ||
| ? dl.some(d => d.enabled !== false) | ||
| : dl?.enabled !== false; | ||
| } |
There was a problem hiding this comment.
Writing tests for functions that aren't imported from the source code is an antipattern. Imagine this: if your code in the .ts file changes and something breaks, your local labelsEnabled implementation in the tests won't change - so the tests will keep passing and won't catch the problem.
| import { describe, it, expect } from 'vitest'; | ||
|
|
||
| // Regression test for #5: the afterDrawDataLabels handler must skip | ||
| // repositioning when dataLabels is disabled, and must handle both the | ||
| // object form and the array form of the dataLabels option. | ||
| // | ||
| // The condition from the plugin, extracted for unit testing: | ||
| // const dl = this.options.dataLabels; | ||
| // const labelsEnabled = Array.isArray(dl) | ||
| // ? (dl as Highcharts.DataLabelsOptions[]).some(d => d.enabled !== false) | ||
| // : dl?.enabled !== false; |
There was a problem hiding this comment.
You know what, you can skip the tests. We don't have integration tests with Highcharts yet, so we can't properly test it anyway.
|
I’ve deleted the test and updated the fix to use H.splat and also added an explicit check for undefined to preserve the original behavior since in Highcharts data labels are enabled by default. |
2 participants
Fixed #5, the chart didn't render without dataLabels enabled.