Fixes #38879 - Update BarChart to Patternfly5 #10743
Conversation
|
Looking at those screenshots, could the chart fill the entire width of the card like it used to? |
kmalyjur
left a comment
kmalyjur
left a comment
There was a problem hiding this comment.
Thank you, I'm leaving some comments.
I haven't tested the chart in the foreman_openscap yet.
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { BarChart as PfBarChart } from 'patternfly-react'; | ||
| import { getBarChartConfig } from '../../../../../services/charts/BarChartService'; |
There was a problem hiding this comment.
The BarChartService.js file could be deleted when it's not used anymore
| noDataMsg, | ||
| config, | ||
| title, | ||
| unloadData, |
| return <MessageBox msg={noDataMsg} icontype="info" />; | ||
|
|
||
| // Configuration based on size | ||
| const getChartDimensions = () => { |
There was a problem hiding this comment.
As Adam noticed, it would be nice if it filled the space
There was a problem hiding this comment.
I do not have data in my bar chart, but this is how it looks filling the space.
| const transformData = rawData => { | ||
| if (!rawData || !Array.isArray(rawData)) return []; | ||
|
|
||
| return rawData.map((item, index) => ({ | ||
| x: item[0], // label | ||
| y: item[1], // value | ||
| name: item[0], | ||
| color: item[2] || undefined, // custom color if provided | ||
| })); | ||
| }; |
There was a problem hiding this comment.
This function could be moved outside of the BarChart component
| x: item[0], // label | ||
| y: item[1], // value | ||
| name: item[0], | ||
| color: item[2] || undefined, // custom color if provided |
There was a problem hiding this comment.
The custom coloring doesn't work
| @@ -1,27 +1,139 @@ | |||
| import { shallow } from 'enzyme'; | |||
| import React from 'react'; | |||
| import { render, screen, fireEvent } from '@testing-library/react'; | |||
| }); | ||
|
|
||
| it('renders empty state when no data provided', () => { | ||
| const { container } = render(<BarChart data={emptyData} noDataMsg="No data available" />); |
There was a problem hiding this comment.
'container' is declared but its value is never read.ts(6133)
The same happens for "renders with custom axis labels" test
|
@kmalyjur Is this ~finished enough to test with packit? |
kmalyjur
left a comment
kmalyjur
left a comment
There was a problem hiding this comment.
Everything is great now, but I forgot that when deleting functions (getBarChartConfig) in foreman core, we need to add a deprecation message first. I'm sorry for not mentioning this before.
There is a deprecation function for that, here is an example of usage: https://github.com/theforeman/foreman/pull/8267/files.
The message could be something like "Please use COMPONENT_NAME from IMPORT_PATH instead".
Could you please not delete getBarChartConfig (and the tests) and deprecate it instead?
Also, the file foreman/config/irbrc_history could be deleted.
|
Thank you, now it just needs to be verified |
|
/packit build |
|
https://copr.fedorainfracloud.org/coprs/fulltext/?fulltext=packit%2Ftheforeman-foreman-10743 is missing, can someone build it please? |
|
/packit build |
5 participants
Fixes: #38879
This PR updates the Bar Chart to use the PF5 components.
API's have been maintained as to not introduce breaking changes to any plugins that may be using the component.
Tests have been updated to use RTL.
Before:
After:
