Fixes #38877 - Update donut chart wrapper to pf5 #10736
Conversation
There was a problem hiding this comment.
initial review:
The failing integration tests will need selectors update probably, they are written in capybara (github action: webpack:compile test:integration apipie:cache - Ruby 2.7 and Node 18).
and the JS tests need to be fixed (JavaScript / test (13, 3.0, 22) )
I tested it on the dashboard where it navigates to a url (no "onclick" prop), and in foreman_remote_execution where it changes the search query with onclick
webpack/assets/javascripts/react_app/components/common/charts/DonutChart/index.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/common/charts/DonutChart/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the DonutChart component from PatternFly React v2 (C3.js-based) to PatternFly 5 (Victory charts-based) while maintaining backward compatibility with existing props and interfaces.
Key Changes:
- Replaced C3.js-based
DonutChartwith PF5's Victory-basedChartDonutcomponent - Added new
getDonutChartConfigPF5function to transform data from C3.js format to Victory format - Updated all tests from Enzyme snapshots to React Testing Library with comprehensive assertions
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
DonutChart/index.js |
Replaced PF2 DonutChart with PF5 ChartDonut, maintained all existing props |
DonutChartService.js |
Updated to call new PF5-specific configuration function |
ChartService.js |
Added getDonutChartConfigPF5 to handle Victory chart data transformation |
ChartService.consts.js |
Added PF5-compatible configuration properties (innerRadius, padding) |
DonutChart.test.js |
Migrated from Enzyme to React Testing Library with 12 comprehensive tests |
DonutChartService.test.js |
Replaced snapshots with 18 assertion-based tests |
ChartBox.test.js |
Updated tests to use React Testing Library |
DonutChart.fixtures.js |
Updated fixtures from C3.js to Victory format |
| Snapshot files | Removed outdated snapshot tests |
| Documentation | Added migration summary and testing documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gitdallas
force-pushed
the
update/DonutChartWrapper-pf5
branch
2 times, most recently
from
bdcf432 to
35fb2d9
Compare
|
The markdown files shouldn't be included in the pull request, should they? |
|
notes for now, planning to clean up some stuff before removing WIP, but i have some failing tests i don't feel are related to changes |
|
The tests that fail now are unrelated |
gitdallas
force-pushed
the
update/DonutChartWrapper-pf5
branch
2 times, most recently
from
257c8f6 to
f608c82
Compare
gitdallas
changed the title
Signed-off-by: Dallas Nicol <[email protected]> add/update testing cleanup Signed-off-by: Dallas Nicol <[email protected]> pr review comments Signed-off-by: Dallas Nicol <[email protected]> update test snaps Signed-off-by: Dallas Nicol <[email protected]>
gitdallas
force-pushed
the
update/DonutChartWrapper-pf5
branch
from
f608c82 to
2bf6a42
Compare
There was a problem hiding this comment.
Thank you, I'm leaving some comments as well.
I see two tooltips when hovering over the chart. Could we leave just one? Also, when the chart was clickable, the mouse pointer used to change. Could it be added again?
All requested changes from @MariaAga were taken care of.
| ); | ||
| const percentage = | ||
| total > 0 | ||
| ? ((maxItem.y / total) * 100).toFixed(title.precision || 1) |
There was a problem hiding this comment.
Even if the title.precision = 0, it will still return 1
| innerRadius: chartConfigForType.innerRadius, | ||
| labels, | ||
| events, | ||
| legendData: undefined, // Can be added if needed |
There was a problem hiding this comment.
Couldn't this line be removed?
| // Mock the ChartDonut component from PatternFly | ||
| jest.mock('@patternfly/react-charts', () => ({ | ||
| ChartDonut: ({ title, subTitle, data }) => ( | ||
| <div data-testid="chart-donut"> | ||
| {title && <div data-testid="chart-title">{title}</div>} | ||
| {subTitle && <div data-testid="chart-subtitle">{subTitle}</div>} | ||
| {data && data.length > 0 && ( | ||
| <div data-testid="chart-data"> | ||
| {data.map((d, i) => ( | ||
| <div key={i} data-testid={`chart-segment-${i}`}> | ||
| {d.x}: {d.y} | ||
| </div> | ||
| ))} | ||
| </div> | ||
| )} | ||
| </div> | ||
| ), | ||
| ChartThemeColor: { | ||
| multiOrdered: 'multiOrdered', | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Would it be please possible to have this test without this mock, so we can test the real component?
| // Mock the DonutChart and BarChart components | ||
| jest.mock('../common/charts/DonutChart', () => ({ | ||
| __esModule: true, | ||
| default: ({ data }) => ( | ||
| <div data-testid="donut-chart"> | ||
| {data && data.length > 0 ? 'Donut Chart with data' : 'Empty donut chart'} | ||
| </div> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock('../common/charts/BarChart', () => ({ | ||
| __esModule: true, | ||
| default: ({ data }) => ( | ||
| <div data-testid="bar-chart"> | ||
| {data && data.length > 0 ? 'Bar Chart with data' : 'Empty bar chart'} | ||
| </div> | ||
| ), | ||
| })); | ||
|
|
||
| jest.mock('../common/Loader', () => ({ | ||
| __esModule: true, | ||
| default: ({ status, children }) => ( | ||
| <div data-testid="loader" data-status={status}> | ||
| {children} | ||
| </div> | ||
| ), | ||
| })); |
There was a problem hiding this comment.
@MariaAga, I'm not sure about those mocks, is it okay to have them like that?
4 participants
fixes: SAT38877
would like someone to test in the plugins, or help me do so. hoping it is good as all the props are unchanged and the charts i'm able to look at seem good.