-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Deploy preview: https://deploy-preview-13471--material-ui-x.netlify.app/ |
location === 'tick' | ||
? timeScale.tickFormat(axis.tickNumber)(v) | ||
: `${v.toLocaleString()}`); | ||
// completedXAxis[axis.id].tickInterval = timeScale.ticks(axis.tickNumber); |
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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
if (axis.scaleType === 'ordinal-time') { | |
if (axis.data[0] instanceof Date) { |
But that would add some kind of magic
🤔 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 We could push that to the user instead too, to fill the gap, but I don't think that is a good idea. |
You can have missing data point by providing Respecting missing data point in time axis is a bit complex, because it's not well defined. Of course, if you have
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 We could provide helpers to generate regular timestamp |
@alexfauquette yeah my idea would be for the users to provide either a function or a |
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 ? |
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 😅 |
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? |
@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. |
const filteredDomain = | ||
(typeof tickInterval === 'function' && domain.filter(tickInterval)) || | ||
(typeof tickInterval === 'object' && tickInterval) || | ||
domain; | ||
return [ | ||
...domain.map((value) => ({ | ||
...filteredDomain.map((value) => ({ |
There was a problem hiding this comment.
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
@@ -236,6 +245,15 @@ function CartesianContextProvider(props: CartesianContextProviderProps) { | |||
? getOrdinalColorScale({ values: axis.data, ...axis.colorMap }) | |||
: getColorScale(axis.colorMap)), | |||
}; | |||
if (axis.data?.[0] instanceof Date) { |
There was a problem hiding this comment.
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
const timeScale = scaleTime(axis.data!, range); | ||
completedXAxis[axis.id].valueFormatter = | ||
axis.valueFormatter ?? | ||
((v, { location }) => | ||
location === 'tick' | ||
? timeScale.tickFormat(axis.tickNumber)(v) | ||
: `${v.toLocaleString()}`); |
There was a problem hiding this comment.
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(...)
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
@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.
time
#13454 the Bar plot is adapted to support the time scaleDate
fixes #12537
fixes #13046
Changelog
The band scale with
Date
data get a better default formater