Fixes #37122 - Update system status chart in job invocations detail page #871

kmalyjur · 2024-01-30T16:17:04Z

Adds system status chart and its legend to the new job invocations detail page.

You can find the discussion here.
Links for filtering the host table through the legend will be added in a future PR.

adamruzicka

The popup ends up being in a bit of a weird place

Could the legend next to the chart be made clickable?

adamruzicka · 2024-02-01T15:58:21Z

webpack/JobInvocationDetail/JobInvocationDetail.scss

+
+// přidat mezeru pod chart když je menší obrazovka
+// ještě style z index.js
+// barva prázdného chart je nějaká čudná


Made me smile when I saw this, well done :)

adamruzicka · 2024-02-01T15:59:12Z

webpack/react_app/components/jobInvocations/AggregateStatus/index.js

-          {statuses.success}
+          {5}


This should go back to the way it was I assume

adamruzicka · 2024-02-01T16:00:03Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+    if (total > 0) return `${succeeded.toString()}/${total}`;
+    if (totalHosts > 0) return `0/${totalHosts}`;
+    return '0';


What's the difference between total and totalHosts?

The total represents the combined count of succeeded, failed, pending, and canceled, whereas totalHosts also includes those currently scheduled in the future.

adamruzicka · 2024-02-01T16:01:44Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+        />
+      </FlexItem>
+      <FlexItem className="chart-legend">
+        <Text className="legend-title">{__('System status')}</Text>


I'm not sure if System is a fortunate wording in this case.

Do you please have any other ideas?

https://www.sketch.com/s/9c533bae-e910-4a9c-89a1-fe67b04a84ca/a/mPmrkMJ
its the word from the design as well

kmalyjur · 2024-02-02T20:22:34Z

@adamruzicka thanks for looking at it!

The popup ends up being in a bit of a weird place

This is the default Patternfly way, but I get that it looks weird. Would it be better if it was on top (when there is only one "color")? Or maybe I could make it follow the mouse?

Could the legend next to the chart be made clickable?

For sure! I thought it should filter out the hosts in the main table, so I wanted to make it when I have the table ready. Is there something else it should do?

MariaAga · 2024-02-05T15:24:48Z

In Progress title is cut off even when there is space

kmalyjur · 2024-02-12T13:54:03Z

In Progress title is cut off even when there is space

@MariaAga Fixed, the width of the chart legend is now adjusted dynamically. I hope it's not too over-complicated.
I also made some changes to the CSS we discussed earlier.

MariaAga · 2024-02-13T11:39:06Z

webpack/JobInvocationDetail/JobInvocationDetail.scss

@@ -0,0 +1,36 @@
+.job-invocation-detail-page-section {
+  .chart-donut {
+    height: 105px;


Please put 105px in a variable in this file

MariaAga · 2024-02-13T15:39:04Z

webpack/JobInvocationDetail/index.js

-              <JobInvocationOverview data={items} />
-            </FlexItem>
+            <Flex
+              style={{ height: '105px' }}


Please move to the .scss file

MariaAga · 2024-02-13T15:44:20Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+  const [legendWidth, setLegendWidth] = useState(270);
+
+  useEffect(() => {
+    const legendContainer = document.querySelector('.chart-legend');


I understand it today, but probably would be best to add a comment on why its needed for future us :)

MariaAga · 2024-02-13T15:45:56Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+                  label: `${d.title} ${d.count} hosts`,
+                  y: d.count,
+                }))
+              : [{ label: `Scheduled: ${totalHosts} hosts`, y: 1 }]


Can you use sprintf here?

MariaAga · 2024-02-13T15:47:18Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+                }))
+              : [{ label: `Scheduled: ${totalHosts} hosts`, y: 1 }]
+          }
+          colorScale={total > 0 ? chartData.map(d => d.color) : ['#8A8D90']}


Please use a pf variable for `#8A8D90``

MariaAga · 2024-02-13T15:47:34Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+          subTitle={__('Systems')}
+          subTitleComponent={
+            <ChartLabel
+              style={{ fontSize: '12px', fill: canceledColor.value }}


Should be in a css file

Should be in a css file

@MariaAga is the comment // inline style overrides PatternFly default styling enough? If so, could you please merge it?

yeah! i approved it, and have to merge permissions in plugins, only @adamruzicka can merge here:)

I didn't want to merge with outstanding comments in case this was still unresolved

MariaAga · 2024-02-13T15:47:37Z

webpack/JobInvocationDetail/JobInvocationSystemStatusChart.js

+            <ChartTooltip pointerLength={0} constrainToVisibleArea />
+          }
+          title={chartDonutTitle}
+          titleComponent={<ChartLabel style={{ fontSize: '20px' }} />}


Should be in a css file

kmalyjur · 2024-02-14T14:21:38Z

@MariaAga thank you, I incorporated everything except for some chart CSS styles (I left comments on those).

MariaAga · 2024-02-15T12:11:25Z

webpack/JobInvocationDetail/JobInvocationDetail.scss

@@ -0,0 +1,42 @@
+$chart_size: 105px;


This also should be in .job-invocation-detail-page-section or have unique name

MariaAga

Looks good!

pnovotny · 2024-03-13T14:31:05Z

QA, LGTM.
The chart status tooltips are centered vertically & horizontally.
The status legend titles are fine too, no text trimming or so. Plus it's aligned dynamically when changing the window width w/o any visible distortions.

pnovotny · 2024-03-13T14:48:10Z

Also WRT UI automation, the identifiers look sufficient to locate all important items.
The main sections - chart, legend and overview - have static IDs and the inner text labels should be locatable with combination of the widget CSS names and text.

adamruzicka · 2024-03-14T08:30:41Z

Thank you @kmalyjur !

kmalyjur · 2024-03-14T08:41:37Z

Thank you @MariaAga @adamruzicka @pnovotny

pr-processor bot added Not yet reviewed Waiting on contributor labels Jan 30, 2024

adamruzicka reviewed Feb 1, 2024

View reviewed changes

kmalyjur force-pushed the job-inv-status-chart branch from 1a314dc to 9da75ee Compare February 5, 2024 10:44

pr-processor bot removed the Waiting on contributor label Feb 5, 2024

kmalyjur changed the title ~~WIP: Fixes #37122 - Update system status chart in job invocations detail page~~ Fixes #37122 - Update system status chart in job invocations detail page Feb 5, 2024

kmalyjur marked this pull request as ready for review February 5, 2024 11:05

kmalyjur force-pushed the job-inv-status-chart branch from 9da75ee to 41a31a2 Compare February 5, 2024 11:26

kmalyjur force-pushed the job-inv-status-chart branch 2 times, most recently from 260e6a9 to d44ac23 Compare February 12, 2024 13:47

MariaAga suggested changes Feb 13, 2024

View reviewed changes

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

kmalyjur force-pushed the job-inv-status-chart branch from d44ac23 to 9183648 Compare February 14, 2024 14:16

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

kmalyjur requested a review from MariaAga February 14, 2024 14:21

MariaAga reviewed Feb 15, 2024

View reviewed changes

kmalyjur force-pushed the job-inv-status-chart branch from 9183648 to d11668b Compare February 19, 2024 09:52

kmalyjur requested a review from MariaAga February 19, 2024 09:53

MariaAga approved these changes Feb 22, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Feb 22, 2024

kmalyjur requested a review from adamruzicka February 28, 2024 08:33

kmalyjur force-pushed the job-inv-status-chart branch 2 times, most recently from 1f7cd15 to 0aa6122 Compare March 4, 2024 10:51

Fixes #37122 - Update system status chart in job invocations detail page

cb8e8ee

kmalyjur force-pushed the job-inv-status-chart branch from 0aa6122 to cb8e8ee Compare March 4, 2024 13:22

adamruzicka approved these changes Mar 14, 2024

View reviewed changes

adamruzicka merged commit 35701a0 into theforeman:master Mar 14, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37122 - Update system status chart in job invocations detail page #871

Fixes #37122 - Update system status chart in job invocations detail page #871

kmalyjur commented Jan 30, 2024 •

edited

Loading

adamruzicka left a comment

adamruzicka Feb 1, 2024

adamruzicka Feb 1, 2024

adamruzicka Feb 1, 2024

kmalyjur Feb 2, 2024

adamruzicka Feb 1, 2024

kmalyjur Feb 2, 2024

MariaAga Feb 5, 2024

kmalyjur commented Feb 2, 2024 •

edited

Loading

MariaAga commented Feb 5, 2024

kmalyjur commented Feb 12, 2024 •

edited

Loading

MariaAga Feb 13, 2024

MariaAga Feb 13, 2024

MariaAga Feb 13, 2024

MariaAga Feb 13, 2024

MariaAga Feb 13, 2024

MariaAga Feb 13, 2024

kmalyjur Mar 13, 2024 •

edited

Loading

MariaAga Mar 13, 2024

adamruzicka Mar 14, 2024

MariaAga Feb 13, 2024

kmalyjur commented Feb 14, 2024

MariaAga Feb 15, 2024

MariaAga left a comment

pnovotny commented Mar 13, 2024

pnovotny commented Mar 13, 2024

adamruzicka commented Mar 14, 2024

kmalyjur commented Mar 14, 2024

Fixes #37122 - Update system status chart in job invocations detail page #871

Fixes #37122 - Update system status chart in job invocations detail page #871

Conversation

kmalyjur commented Jan 30, 2024 • edited Loading

adamruzicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmalyjur commented Feb 2, 2024 • edited Loading

MariaAga commented Feb 5, 2024

kmalyjur commented Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmalyjur Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmalyjur commented Feb 14, 2024

Choose a reason for hiding this comment

MariaAga left a comment

Choose a reason for hiding this comment

pnovotny commented Mar 13, 2024

pnovotny commented Mar 13, 2024

adamruzicka commented Mar 14, 2024

kmalyjur commented Mar 14, 2024

kmalyjur commented Jan 30, 2024 •

edited

Loading

kmalyjur commented Feb 2, 2024 •

edited

Loading

kmalyjur commented Feb 12, 2024 •

edited

Loading

kmalyjur Mar 13, 2024 •

edited

Loading