Fixes #39070 - Update AreaChart to PatternFly5#10803
Fixes #39070 - Update AreaChart to PatternFly5#10803tlabaj wants to merge 1 commit intotheforeman:developfrom
Conversation
tlabaj
changed the title
tlabaj
force-pushed
the
areachart
branch
3 times, most recently
from
7395e53 to
f0cc83c
Compare
kmalyjur
left a comment
kmalyjur
left a comment
There was a problem hiding this comment.
I'm leaving an initial review, I might comment more tomorrow.
|
|
||
| return ( | ||
| <div style={{ height: chartHeight, width: chartWidth || '100%' }}> | ||
| <Chart |
There was a problem hiding this comment.
Could the chart fill the entire width of the card?
| legendData={legendData} | ||
| legendOrientation="horizontal" | ||
| legendPosition="bottom" | ||
| legendComponent={<ChartLegend />} |
There was a problem hiding this comment.
The old chart has clickable legend text, it would be nice to have it in the new one, too.
| <ChartAxis | ||
| tickFormat={t => { | ||
| if (config === 'timeseries' && t instanceof Date) { |
There was a problem hiding this comment.
The config === 'timeseries' can be removed since the data transformation always creates Date objects anyway.
| <ChartAxis | |
| tickFormat={t => { | |
| if (config === 'timeseries' && t instanceof Date) { | |
| <ChartAxis | |
| tickFormat={t => { | |
| if (t instanceof Date) { |
| expect(chartWrapper).toHaveStyle({ height: '300px', width: '500px' }); | ||
| }); | ||
|
|
||
| it('renders with onclick callback', () => { |
There was a problem hiding this comment.
The test checks if the chart renders, but it does not verify if the callback actually triggers when clicked.
| const handleClick = useMemo( | ||
| () => (event, datum) => { | ||
| if (onclick && datum && datum.name) { | ||
| onclick({ id: datum.name }); | ||
| } | ||
| }, | ||
| [onclick] | ||
| ); |
There was a problem hiding this comment.
I think this function is broken or not called.
| height={chartHeight} | ||
| width={chartWidth} | ||
| padding={padding} | ||
| themeColor={ChartThemeColor.orange} |
There was a problem hiding this comment.
Could I please ask why orange was chosen?
There was a problem hiding this comment.
The old chart was orange. I can change it to just be the default.
| } | ||
|
|
||
| const timestamps = timeColumn.slice(1); | ||
| const dates = timestamps.map(epochSecs => new Date(epochSecs * 1000)); |
There was a problem hiding this comment.
The axis shows raw numbers (like "17709"). There could be a check if the backend is already sending milliseconds, and if so, it shouldn't use * 1000.
2 participants
Fixes: #39070
This PR updates the Area Chart to use PF5 components.