Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[charts] Fix Barchart with empty dataset throwing an error #12708

Conversation

JCQuintas
Copy link
Member

We just use optional chain and nullchecks as fallbacks.

@alexfauquette Should we add tests for these functions?

Fixes #12462

@JCQuintas JCQuintas added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels Apr 8, 2024
@mui-bot
Copy link

mui-bot commented Apr 8, 2024

Deploy preview: https://deploy-preview-12708--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against e7afe4e

@alexfauquette
Copy link
Member

I agree it fixes the issue, but I'm wondering why do we have this issue with bar chart and not with line chart? They are super close, so they should have the same behavior with empty dataset.

For the fallback on [null, null] I'm ok, it's the same behavior, but for the use of optional chain I'm wondering if we are missing something since I don't see them used in the line chart

@JCQuintas
Copy link
Member Author

JCQuintas commented Apr 9, 2024

@alexfauquette In the Line chart it is divided into two functions and we check for

https://github.com/JCQuintas/mui-x/blob/3cee37ded0a82ab1b2854fa6f2a430c29c32f090/packages/x-charts/src/LineChart/extremums.ts#L17-L19

if (stackedData.length === 0) {
    return [null, null];
}

also the first ?. there can be removed, the issue only happens because data doesn't exist

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Ok, if you want to add test, their is a similar one in the LineChart folder. The only test existing for now

@JCQuintas
Copy link
Member Author

Added tests, odd behaviour is that vertical layout returns [Infinity, -Infinity] while horizontal is returning [null, null]. Doesn't seem to affect anything though

@alexfauquette
Copy link
Member

Not that surprising. It comes from Math.min() returning Infinity.

const getBaseExtremum: ExtremumGetter<'bar'> = (params) => {
  const { axis } = params;

  const minX = Math.min(...(axis.data ?? []));
  const maxX = Math.max(...(axis.data ?? []));
  return [minX, maxX];
};

What is more surprising is that those extremes are computed. For "band" and "point" scales, the notion of extreme values does not make sense because they contain categories. So they do not have an notion of order.

In cartesian context, those scales got an early return before the min/max values are used. So they could return wherever they want.

Maybe the correct behavior would be that in vertical it always returns [null, null] but since this value is never used, I don't have strong opinion about it

@JCQuintas JCQuintas merged commit 9d049bb into mui:master Apr 9, 2024
17 checks passed
};
};

describe('BarChart - extremums', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Time will tell if this test isn't too low level. Testing public APIs tends to scale better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Charts] Barchart with empty dataset should render an empty chart
4 participants