Fixes #39070 - Update AreaChart to PatternFly5 by tlabaj · Pull Request #10803 · theforeman/foreman

tlabaj · 2025-12-17T14:24:27Z

Fixes: #39070

This PR updates the Area Chart to use PF5 components.

New images after latest Updates

Before:

After:

kmalyjur

I'm leaving an initial review, I might comment more tomorrow.

kmalyjur · 2026-02-17T17:27:22Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+
+  return (
+    <div style={{ height: chartHeight, width: chartWidth || '100%' }}>
+      <Chart


Could the chart fill the entire width of the card?

kmalyjur · 2026-02-17T17:38:28Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+        legendData={legendData}
+        legendOrientation="horizontal"
+        legendPosition="bottom"
+        legendComponent={<ChartLegend />}


The old chart has clickable legend text, it would be nice to have it in the new one, too.

kmalyjur · 2026-02-17T18:40:58Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+        <ChartAxis
+          tickFormat={t => {
+            if (config === 'timeseries' && t instanceof Date) {


The config === 'timeseries' can be removed since the data transformation always creates Date objects anyway.

Suggested change

<ChartAxis

tickFormat={t => {

if (config === 'timeseries' && t instanceof Date) {

<ChartAxis

tickFormat={t => {

if (t instanceof Date) {

kmalyjur · 2026-02-17T18:43:48Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/AreaChart.test.js

+    expect(chartWrapper).toHaveStyle({ height: '300px', width: '500px' });
+  });
+
+  it('renders with onclick callback', () => {


The test checks if the chart renders, but it does not verify if the callback actually triggers when clicked.

kmalyjur

This is how I see before and after:

kmalyjur · 2026-02-18T13:21:48Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+  const handleClick = useMemo(
+    () => (event, datum) => {
+      if (onclick && datum && datum.name) {
+        onclick({ id: datum.name });
+      }
+    },
+    [onclick]
+  );


I think this function is broken or not called.

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

kmalyjur · 2026-02-18T13:35:29Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+    }
+
+    const timestamps = timeColumn.slice(1);
+    const dates = timestamps.map(epochSecs => new Date(epochSecs * 1000));


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.

tlabaj · 2026-02-26T20:12:55Z

@kmalyjur Could you please re-review this one?

kmalyjur

This is just an initial review, I'll look at it again on Monday.

It looks great now, the time is in good format, but it is overlapped and not readable:

kmalyjur · 2026-02-27T19:00:06Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

  onclick,
  noDataMsg,
  config,
-  unloadData,


I'm so sorry, I think I told you before that it's unused, which it is, but I forgot about: "Received props should be kept the same, otherwise this task should also refactor the chart uses in the plugin."
It should be there, and the default should be false.

kmalyjur · 2026-03-10T18:17:06Z

Thank you, the code looks right now. The only thing that works/looks different from the old chart is the timestamps. I explain how I understand it in the comment above.

kmalyjur · 2026-03-11T10:17:36Z

LGTM, I need to verify it now

kmalyjur · 2026-03-16T15:12:05Z

I noticed a few more things that may need fixing.

There is the same issue as in BarChart with the "Time in seconds" text and the y-axis.
The x-axis text could be longer and not readable - a tooltip for longer text could be used. EDIT: Although we could assume it's not that long in our case, so my comment might be irrelevant.
When hovering over the color square in the legend, the mouse looks like it should be clickable, but I think it isn't - should it be doing anything?

kmalyjur · 2026-03-18T19:30:47Z

@tlabaj I have the same comment on it as on the BarChart PR - could the padding not be hardcoded? I worry it would be overlapping again for longer y-axis values.

kmalyjur · 2026-03-24T13:21:47Z

@tlabaj Thank you for your changes, it looks good.
I left this comment (#10743 (comment)) for the BarChart, the logic could be used for AreaChart, too.

kmalyjur

This label is cut off:

kmalyjur · 2026-03-27T17:53:17Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+          axisLabelComponent={
+            yAxisLabel ? (
+              <ChartLabel x={Y_AXIS_LABEL} dy={yAxisLabelOffset} />
+            ) : undefined


Linter error

kmalyjur · 2026-03-27T17:54:57Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/AreaChartLegend.js

+    : t;
+};
+
+/** Match BarChart: fixed decimals for normal range; scientific notation for very large magnitudes. */


Great comment, but "Match BarChart" is unnecessary

kmalyjur · 2026-03-27T17:55:45Z

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

+const DEFAULT_HEIGHT = 250;
+const DEFAULT_WIDTH = 800;
+
+/** Left padding is computed from y-axis tick label width (see BarChart). */


Good comment, but "(see BarChart)" seems unnecessary

kmalyjur

This fixes the label. Other than that, it's ready to get merged.

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js

Co-authored-by: kmalyjur <78563507+kmalyjur@users.noreply.github.com> Made-with: Cursor

pr-processor bot added Not yet reviewed Waiting on contributor labels Dec 17, 2025

github-actions bot added the UI label Dec 17, 2025

tlabaj force-pushed the areachart branch from 4d68513 to a5bf69a Compare December 17, 2025 15:06

tlabaj force-pushed the areachart branch from a5bf69a to 29bf205 Compare February 10, 2026 15:32

tlabaj changed the title ~~WIP: Update Area chart to PF5~~ Fixes #39070 - Update AreaChart to PatternFly5 Feb 10, 2026

tlabaj marked this pull request as ready for review February 10, 2026 15:34

tlabaj force-pushed the areachart branch from 29bf205 to b9d2fc3 Compare February 10, 2026 15:47

pr-processor bot removed the Waiting on contributor label Feb 10, 2026

tlabaj force-pushed the areachart branch 4 times, most recently from f0cc83c to d891a09 Compare February 10, 2026 16:26

kmalyjur suggested changes Feb 17, 2026

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Feb 17, 2026

kmalyjur suggested changes Feb 18, 2026

View reviewed changes

tlabaj force-pushed the areachart branch from d891a09 to 6bf38e3 Compare February 24, 2026 20:46

pr-processor bot added Needs re-review and removed Waiting on contributor labels Feb 24, 2026

tlabaj force-pushed the areachart branch 6 times, most recently from aff21d3 to 29cbd88 Compare February 26, 2026 20:10

tlabaj force-pushed the areachart branch from 29cbd88 to 3cba518 Compare February 26, 2026 20:29

kmalyjur suggested changes Feb 27, 2026

View reviewed changes

pr-processor bot added the Waiting on contributor label Feb 27, 2026

pr-processor bot removed the Needs re-review label Mar 9, 2026

tlabaj force-pushed the areachart branch from 6ae29f2 to e1cbdf5 Compare March 9, 2026 18:32

tlabaj force-pushed the areachart branch 3 times, most recently from 2a86311 to f459f34 Compare March 10, 2026 19:33

tlabaj force-pushed the areachart branch from f459f34 to 888a8a7 Compare March 16, 2026 20:29

tlabaj force-pushed the areachart branch 5 times, most recently from 554dd27 to 7d45eda Compare March 23, 2026 14:50

tlabaj force-pushed the areachart branch 2 times, most recently from 2a383dd to efcd088 Compare March 25, 2026 14:53

kmalyjur suggested changes Mar 27, 2026

View reviewed changes

pr-processor bot added the Waiting on contributor label Mar 27, 2026

tlabaj force-pushed the areachart branch from efcd088 to 9419179 Compare March 27, 2026 20:14

pr-processor bot added Needs re-review and removed Waiting on contributor labels Mar 27, 2026

kmalyjur approved these changes Apr 8, 2026

View reviewed changes

webpack/assets/javascripts/react_app/components/common/charts/AreaChart/index.js Outdated Show resolved Hide resolved

pr-processor bot added Waiting on contributor and removed Needs re-review labels Apr 8, 2026

Fixes #39070 - Update AreaChart to PatternFly5

6af6288

Co-authored-by: kmalyjur <78563507+kmalyjur@users.noreply.github.com> Made-with: Cursor

tlabaj force-pushed the areachart branch from fce1a47 to 6af6288 Compare April 8, 2026 14:37

pr-processor bot added Needs re-review and removed Waiting on contributor labels Apr 8, 2026

Conversation

tlabaj commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmalyjur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlabaj commented Feb 26, 2026

Uh oh!

kmalyjur left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur commented Mar 10, 2026

Uh oh!

kmalyjur commented Mar 11, 2026

Uh oh!

kmalyjur commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmalyjur commented Mar 18, 2026

Uh oh!

kmalyjur commented Mar 24, 2026

Uh oh!

kmalyjur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmalyjur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tlabaj commented Dec 17, 2025 •

edited

Loading

kmalyjur left a comment •

edited

Loading

kmalyjur left a comment •

edited

Loading

kmalyjur commented Mar 16, 2026 •

edited

Loading