-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix datachart bug for single chart data #7020
Conversation
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.
The DataChart works with one value when direction="vertical"
, but when we change direction
to "horizontal that is where we run into issues. This makes me think that there is something in the YAxis code that is not handling the 1 data point case well. The XAxis code seems to be able to handle 1 data point. Comparing the XAxis code with the YAxis code might be a good place to look into more.
@@ -547,7 +547,8 @@ const DataChart = forwardRef( | |||
if (!property || (!horizontal && y) || (horizontal && !y)) { | |||
if (render) return render(value); | |||
} else { | |||
const datum = data[axisValue]; | |||
// const datum = data[axisValue]; | |||
const datum = data[0]; |
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.
This change causes an issue with the axis labels. If we have the following code we end up with two xaxis labels that are the same value (Jun 23) coming from data[0].
const data = [
{ a: 1, b: 'one', c: 111111, d: '2020-06-24' },
{ a: 2, b: 'two', c: 222222, d: '2020-06-23' },
];
export const Simple = () => (
<Box align="center" justify="start" pad="large">
<DataChart
data={data}
series={['d', 'a']}
axis={{
x: { property: 'd' },
y: { property: 'a' },
}}
/>
</Box>
);
@@ -527,7 +527,7 @@ const DataChart = forwardRef( | |||
series.forEach(({ property, render }) => { | |||
if ( | |||
!render && | |||
data.length > 1 && | |||
data.length >= 1 && |
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.
data.length >= 1 && | |
data.length > 1 && |
I think we should revert this change back. If we only have 1 data point we don't need the createDateFormat code
if (delta === 0) | ||
// equal 0 - single data point | ||
options = { | ||
month: full ? 'short' : 'numeric', | ||
day: 'numeric', | ||
}; | ||
else if (delta < 60000) |
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.
if (delta === 0) | |
// equal 0 - single data point | |
options = { | |
month: full ? 'short' : 'numeric', | |
day: 'numeric', | |
}; | |
else if (delta < 60000) | |
if (delta < 60000) |
Lets revert this change back, I don't think we need to adjust this
@@ -96,7 +102,7 @@ export const createDateFormat = (firstValue, lastValue, full) => { | |||
else if (delta < 86400000) | |||
// less than 1 day | |||
options = { hour: 'numeric' }; | |||
else if (delta < 2592000000) | |||
else if (delta < 2592000000 || delta === 0) |
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.
else if (delta < 2592000000 || delta === 0) | |
else if (delta < 2592000000) |
Let's revert this change as well
Closing due to inactivity, feel free to reopen |
What does this PR do?
This pull request is meant to fix datachart bug with single data item
Where should the reviewer start?
What testing has been done on this PR?
How should this be manually tested?
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
closes #6991
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
Backward compatible