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] Support BarChart with Date data #13471

Merged
merged 6 commits into from
Jun 26, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jun 13, 2024

@JCQuintas I was looking at your PR #13454 and I thought about another way of doing it

https://codesandbox.io/p/sandbox/zealous-morning-9lw243?file=%2Fsrc%2FDemo.tsx%3A5%2C1

This PR try to go the other way.

  • In [charts] POC Support BarChart with scaletype time #13454 the Bar plot is adapted to support the time scale
  • In this one we create a new axis type that generate a band scale manipulating date. We could also make it kind of magic by keeping band type and just checking if the first data element is a Date

fixes #12537
fixes #13046

Changelog

The band scale with Date data get a better default formater

@mui-bot
Copy link

mui-bot commented Jun 13, 2024

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

Generated by 🚫 dangerJS against 06d6827

location === 'tick'
? timeScale.tickFormat(axis.tickNumber)(v)
: `${v.toLocaleString()}`);
// completedXAxis[axis.id].tickInterval = timeScale.ticks(axis.tickNumber);
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to be sure the generated tick exist in the data

@@ -224,6 +224,16 @@ function CartesianContextProvider(props: CartesianContextProviderProps) {
? getOrdinalColorScale({ values: axis.data, ...axis.colorMap })
: getColorScale(axis.colorMap)),
};
if (axis.scaleType === 'ordinal-time') {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could skip the craetion of the special scaletype with

Suggested change
if (axis.scaleType === 'ordinal-time') {
if (axis.data[0] instanceof Date) {

But that would add some kind of magic

@JCQuintas
Copy link
Member

🤔 I think it can make a lot of sense to create this as a separate axis type, but I would probably want it also respect missing data points https://codesandbox.io/p/sandbox/zealous-morning-9lw243

It should behave like:
Screenshot 2024-06-13 at 16 27 44

Instead of:
Screenshot 2024-06-13 at 16 27 58

We could push that to the user instead too, to fill the gap, but I don't think that is a good idea.

@alexfauquette
Copy link
Member Author

alexfauquette commented Jun 13, 2024

I would probably want it also respect missing data points

You can have missing data point by providing null values. But you need to provide the x value.
https://mui.com/x/react-charts/lines/#skip-missing-points

image

Respecting missing data point in time axis is a bit complex, because it's not well defined.

Of course, if you have

  • 1, 2, 3, 4, 10, 11, 12 you can expect to have a bar with width=1
  • 2, 4, 6, 10, 12 you can expect to have a bar with width=2
  • 2, 3, 4, 6, 10, 12 you should probably have a bar with width=1 but with lots of holes

To do so we will have to compute what is the minimal distance between two consecutive values, expecting it to be a common divider.

In #13454 you overcome this difficulty by using timeDay but it does not work with the step between each point is 12h, 1h, 30min, 15min, ...

We could provide helpers to generate regular timestamp

@JCQuintas
Copy link
Member

@alexfauquette yeah my idea would be for the users to provide either a function or a time|hour|minute|... string we can generate the correct data.

@alexfauquette
Copy link
Member Author

I was think about something like

<BarChart
	xAxis={[{
		scale: 'ordinal-time',
		data: timeGenerator({
			from: new Date(2023, 5, 10),
			to: new Date(2023, 6, 15),
			step: 24 * 3600 * 1000
		})
	{/* ... */}
/>

But not convinced it's necessary. I assume if you have some data at regular intervals, you also have the time stamps.

@Janpot do you have some insight on this aspect: how to handle missing, or irregular time stamp for bar chart ?

@Janpot
Copy link
Member

Janpot commented Jun 15, 2024

I assume the common use case would be grouped/aggregated data, e.g. revenue per day, or # of errors per hour. So fixed intervals, where missing data means 0, no bar. I'm not really sure how I'd translate that into an API.

@JCQuintas
Copy link
Member

I assume the common use case would be grouped/aggregated data, e.g. revenue per day, or # of errors per hour. So fixed intervals, where missing data means 0, no bar. I'm not really sure how I'd translate that into an API

If you are consuming APIs you probably have these options. But real time data, as logs and IoT wouldn't have it in a consistent way.

We could always just add this and wait for extra input there too 😅

@Janpot
Copy link
Member

Janpot commented Jun 17, 2024

But real time data, as logs and IoT wouldn't have it in a consistent way.

But how do you show raw realtime data in a bar chart? You'd always do some sort of bucketing in fixed intervals. What else would be the meaning of "a bar" on a time scale? Do you have a real world use-case of a bar chart with a time scale where the bars don't represent the same interval?

@JCQuintas
Copy link
Member

@Janpot yeah, I agree with what you are saying, my point was more if we should provide the user with the means to aggregate the data and display it, or if the user should handle that themselves.

@Janpot
Copy link
Member

Janpot commented Jun 17, 2024

@Janpot yeah, I agree with what you are saying, my point was more if we should provide the user with the means to aggregate the data and display it, or if the user should handle that themselves.

👍 got it. Perhaps it could be valuable to have built in aggregation in the "dataSet+dataKey" part of the API. And leave the option for raw definition in the "data" part of the API. To sort of provide the option for both a low and high level abstraction. Not sure, feels like something I would use. Defining the data as an aggregation feels like a more error-tolerant way of dealing with duplicate or missing X values, it would become a non-issue.

@JCQuintas JCQuintas changed the title Alternative PoC [charts] Support BarChart with Date data Jun 17, 2024
@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Jun 18, 2024
@alexfauquette alexfauquette marked this pull request as ready for review June 18, 2024 13:10
Comment on lines +104 to +109
const filteredDomain =
(typeof tickInterval === 'function' && domain.filter(tickInterval)) ||
(typeof tickInterval === 'object' && tickInterval) ||
domain;
return [
...domain.map((value) => ({
...filteredDomain.map((value) => ({
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit out of scope, but it allows filtering tickets for band scale

@alexfauquette alexfauquette requested a review from JCQuintas June 18, 2024 13:12
@@ -236,6 +245,15 @@ function CartesianContextProvider(props: CartesianContextProviderProps) {
? getOrdinalColorScale({ values: axis.data, ...axis.colorMap })
: getColorScale(axis.colorMap)),
};
if (axis.data?.[0] instanceof Date) {
Copy link
Member

Choose a reason for hiding this comment

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

Would a isDateData function work here?

const isDateData = (data?: any[]): data is Date[] => data?.[0] instanceof Date

Comment on lines 249 to 255
const timeScale = scaleTime(axis.data!, range);
completedXAxis[axis.id].valueFormatter =
axis.valueFormatter ??
((v, { location }) =>
location === 'tick'
? timeScale.tickFormat(axis.tickNumber)(v)
: `${v.toLocaleString()}`);
Copy link
Member

Choose a reason for hiding this comment

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

We could probably also create a dateValueFormatter and just completedXAxis[axis.id].valueFormatter = dateValueFormatter(...)

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 19, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 21, 2024
@oscar-b
Copy link

oscar-b commented Jan 7, 2025

@alexfauquette Hi! Was this change reverted? Using version latest in your linked sandbox doesn't work: https://codesandbox.io/p/sandbox/distracted-nightingale-rqcznv?file=/src/Demo.tsx:6,15

Or can this be achieved some other way now? Also scaleType: "ordinal-time" doesn't seem to exist? I'm so confused.

MUI X: The x-axis with id "defaultized-x-axis-0" should be of type "band" to display the bar series of id "auto-generated-id-0".

image

It does seem that just using scaleType: 'band' with an x-axis containing Date actually works, but they are formatted with a 12h time format? LocalizationProvider is setup but doesn't seem to be used by charts?

#11374 but then I'd have to re-implement the logic that MUI provides which selectively shows date at the beginning of each day and so on?

Talking to myself here but wasn't that hard:

image
valueFormatter: (v, context) => {
	switch (context.location) {
		case 'legend':
		case 'tooltip': {
			const date = dayjs(v).startOf('hour');

			return `${date.format('YYYY-MM-DD HH:mm')} - ${date.add(1, 'hour').format('HH:mm')}`;
		}

		case 'tick': {
			const date = dayjs(v);

			return date.hour() === 0
				? date.format('dd DD')
				: dayjs(v).format('HH');
		}
	}
},

@alexfauquette
Copy link
Member Author

alexfauquette commented Jan 7, 2025

You're confused because you look at codesandbox created for discussion. The idea of scaleType: "ordinal-time" has been dropped in favor of detecting band scale with Date in its data.

The feature is a best effort of guessing the value format according to the data. D3 does some computation to know if according to the number of ticks and the range of data it should show dates or hours. The codesandbox (when fixed by removing the "ordinal-time") shows the axis for a graphe where point are increased hour per hour, or day by day. ANd the axis addpts by either plotting only the date "Tue 06" or the hour "05 AM".

image

It's not perfect, we could crease the number of tick to better shouw which tick is associated to the label, or other improvement, but at least it does not display the full date+time.

I'd have to re-implement the logic that MUI provides which selectively shows date at the beginning of each day and so on?

Formatting is much easier to do on your side, because you know which type of data you're expecting and what is important in it.

If you still have questions, or improvement suggestion, please open another issue such that we can track progress on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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] Support for ordinal axes [Charts] Support BarChart with scaleType: time
5 participants