[charts] Fix default axis highlight for axes without data attribute #18985
Conversation
|
Deploy preview: https://deploy-preview-18985--material-ui-x.netlify.app/ Bundle size report
|
| {xAxisHighlight && xAxisHighlight !== 'none' && ( | ||
| <ChartsXHighlight type={xAxisHighlight} classes={classes} /> | ||
| )} | ||
| {yAxisHighlight && yAxisHighlight !== 'none' && ( | ||
| <ChartsYHighlight type={yAxisHighlight} classes={classes} /> | ||
| )} |
There was a problem hiding this comment.
While debugging, I noticed 'none' renders the component which is bad because we call the x/y-axis highlight selector to simply return null at the end
CodSpeed Performance ReportMerging #18985 will not alter performanceComparing Summary
|
| @@ -181,4 +182,29 @@ describe('useChartCartesianAxis - axis highlight', () => { | |||
| ]); | |||
| }, | |||
| ); | |||
|
|
|||
| it.skipIf(isJSDOM)('should allow to highlight axes without data', async () => { | |||
There was a problem hiding this comment.
Should we have a test case for a continuous scale as well? The implementation seems to be different for discrete and continuous scales.
There was a problem hiding this comment.
Good idea. I added a test for the x-axis which is a band scale 👍
There was a problem hiding this comment.
This test uses a band scale, but the new one as well. Shouldn't the new one use a continuous scale (e.g., linear)?
There was a problem hiding this comment.
Both tests have the same chart config.
- The first one display y-axis highlight (on a continuous scale without data)
- The first one display x-axis highlight (on a band scale with data)
We could also have "continuous scale without data" but "band scale without data" does not exist
There was a problem hiding this comment.
Ah, you're right. Missed that 👍
| if (!isBandScale(scale)) { | ||
| const value = scale.invert(pointerValue); | ||
|
|
||
| if (dataIndex < 0) { | ||
| if (dataIndex === null) { | ||
| return value; | ||
| } | ||
| return axisData![dataIndex]; | ||
| } |
There was a problem hiding this comment.
We can avoid inverting if dataIndex !== null:
| if (!isBandScale(scale)) { | |
| const value = scale.invert(pointerValue); | |
| if (dataIndex < 0) { | |
| if (dataIndex === null) { | |
| return value; | |
| } | |
| return axisData![dataIndex]; | |
| } | |
| if (!isBandScale(scale)) { | |
| if (dataIndex === null) { | |
| return scale.invert(pointerValue); | |
| } | |
| return axisData![dataIndex]; | |
| } |
| }); | ||
| }); | ||
|
|
||
| it.skipIf(isJSDOM)('should allow to highlight axes with data', async () => { |
There was a problem hiding this comment.
optional: all the tests in this file are using skipIf, we can move the skipIf to the top describe if we want. describe.skipIf(isJSDOM)
While adding the controlled version, I took a bad shortcut: I assumed if no axis index get is available, then no value corresponds to the current pointer.