pr review comments

Signed-off-by: Dallas Nicol <[email protected]>

Author: gitdallas

DONUT_CHART_PF5_MIGRATION_SUMMARY.md

@@ -42,15 +42,14 @@ Successfully migrated the DonutChart component from PatternFly React v2 (C3.js-b
 - ✅ Updated padding for better chart display
 - ✅ Maintained backward compatibility by keeping C3.js properties for reference
 
-## Props Interface - UNCHANGED ✅
-All existing props work the same:
+## Props Interface ✅
+All existing props work the same (except unloadData which was removed):
 ```javascript
 {
   data: Array,              // [['Label', value, color], ...]
   config: String,           // 'regular' | 'medium' | 'large'
   noDataMsg: String,        // Custom empty state message
   title: Object,            // { type: 'percent', precision: 1 } or { primary, secondary }
-  unloadData: Boolean,      // Kept for compatibility (not used in PF5)
   onclick: Function,        // Click handler
   searchUrl: String,        // URL for search navigation
   searchFilters: Object,    // Search filter mapping
@@ -126,10 +125,12 @@ const percentage = ((maxValue / total) * 100).toFixed(precision);
 
 
 ### ⚠️ Known Differences
-1. **unloadData prop**: PF5 doesn't support `unloadBeforeLoad` - prop is accepted but ignored
-2. **Legend**: Large config legend may need styling adjustments
-3. **Tooltip Styling**: Victory tooltips have different visual styling than C3.js (but show same percentage information)
-4. **Animations**: PF5 charts have built-in animations (C3.js animations were configurable)
+1. **unloadData prop**: Removed (PF5 doesn't support `unloadBeforeLoad`)
+2. **Label truncation**: Labels are no longer truncated at 30 characters (was not visible in UI)
+3. **Legend**: Large config legend may need styling adjustments
+4. **Tooltip Styling**: Victory tooltips have different visual styling than C3.js (but show same percentage information)
+5. **Animations**: PF5 charts have built-in animations (C3.js animations were configurable)
+6. **Chart wrapper**: Added `margin: auto` for proper centering
 
 ## Configuration Mapping
 
@@ -155,4 +156,29 @@ const percentage = ((maxValue / total) * 100).toFixed(precision);
 
 **Can be removed eventually (after all charts migrated):**
 - `patternfly-react` ^2.40.0 (still needed for Bar/Line/Area charts)
-- `patternfly` ^3.59.5 (still needed for C3.js charts)
+- `patternfly` ^3.59.5 (still needed for C3.js charts)
+
+## PR Review Changes
+
+### Changes based on code review feedback:
+
+1. **Removed `unloadData` prop** 
+   - This prop was not used by any components
+   - PF5 doesn't support `unloadBeforeLoad` anyway
+   - Files updated: DonutChart/index.js
+
+2. **Added CSS centering**
+   - Added `margin: auto` to wrapper div style
+   - Ensures chart is centered in all contexts (including /legacy/job_invocations)
+   - Files updated: DonutChart/index.js
+
+3. **Fixed integration test selector**
+   - Added `.donut-chart-pf` class back to wrapper div for tests
+   - Files updated: DonutChart/index.js
+
+4. **Removed unused label truncation logic**
+   - Removed shortLabel logic that truncated labels at 30 characters
+   - This truncation was not visible anywhere in the UI (no legend displayed)
+   - Simplified code by using full labels directly
+   - Updated tests to reflect the change
+   - Files updated: ChartService.js, DonutChartService.test.js

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

@@ -12,7 +12,6 @@ const DonutChart = ({
   config,
   noDataMsg,
   title,
-  unloadData, // Note: PF5 doesn't have unloadBeforeLoad, this prop is kept for compatibility but not used
   searchUrl,
   searchFilters,
 }) => {
@@ -62,7 +61,7 @@ const DonutChart = ({
     }
 
     return (
-      <div style={{ height, width }}>
+      <div className="donut-chart-pf" style={{ height, width, margin: 'auto' }}>
         <ChartDonut
           ariaDesc="Donut chart"
           ariaTitle={subtitleText || 'Chart'}
@@ -91,7 +90,6 @@ DonutChart.propTypes = {
   config: PropTypes.oneOf(['regular', 'medium', 'large']),
   noDataMsg: PropTypes.string,
   title: PropTypes.object,
-  unloadData: PropTypes.bool,
   onclick: PropTypes.func,
   searchUrl: PropTypes.string,
   searchFilters: PropTypes.object,
@@ -102,7 +100,6 @@ DonutChart.defaultProps = {
   config: 'regular',
   noDataMsg: __('No data available'),
   title: { type: 'percent', precision: 1 },
-  unloadData: false,
   onclick: noop,
   searchUrl: undefined,
   searchFilters: undefined,

webpack/assets/javascripts/services/charts/ChartService.js

@@ -124,15 +124,10 @@ export const getDonutChartConfigPF5 = ({
   // Transform data from [label, value, color] to Victory format
   const transformedData = [];
   const colorScale = [];
-  const longNames = {};
 
   data.forEach(item => {
     const [label, value, color] = item;
-    const shortLabel =
-      label.length > 30 ? `${label.substring(0, 27)}...` : label;
-
-    longNames[shortLabel] = label;
-    transformedData.push({ x: shortLabel, y: value, name: label });
+    transformedData.push({ x: label, y: value, name: label });
 
     if (color) {
       colorScale.push(color);
@@ -175,9 +170,8 @@ export const getDonutChartConfigPF5 = ({
 
   // Configure labels to show full name and percentage in tooltip
   const labels = ({ datum }) => {
-    const fullName = longNames[datum.x] || datum.x;
     const percentage = total > 0 ? ((datum.y / total) * 100).toFixed(1) : 0;
-    return `${fullName}: ${percentage}%`;
+    return `${datum.name}: ${percentage}%`;
   };
 
   return {

webpack/assets/javascripts/services/charts/DonutChartService.test.js

@@ -107,8 +107,8 @@ describe('getDonutChartConfig', () => {
     });
   });
 
-  describe('label truncation', () => {
-    it('truncates labels longer than 30 characters', () => {
+  describe('label handling', () => {
+    it('preserves full labels including long ones', () => {
       const result = getDonutChartConfig({
         data: inputDataWithLongLabels,
         onclick: jest.fn(),
@@ -120,17 +120,14 @@ describe('getDonutChartConfig', () => {
         item.name.includes('extra extra extra extra extra extra extra')
       );
 
-      // x should be truncated
-      expect(longLabelItem.x.length).toBeLessThanOrEqual(30);
-      expect(longLabelItem.x).toMatch(/\.\.\.$/);
-
-      // name should preserve full label
+      // Both x and name should contain the full label
+      expect(longLabelItem.x).toBe(longLabelItem.name);
       expect(longLabelItem.name).toContain(
         'extra extra extra extra extra extra extra'
       );
     });
 
-    it('preserves labels shorter than 30 characters', () => {
+    it('preserves all labels as-is', () => {
       const result = getDonutChartConfig({
         data: inputData,
         onclick: jest.fn(),
@@ -139,9 +136,7 @@ describe('getDonutChartConfig', () => {
       });
 
       result.data.forEach(item => {
-        if (item.name.length <= 30) {
-          expect(item.x).toBe(item.name);
-        }
+        expect(item.x).toBe(item.name);
       });
     });
 
@@ -252,8 +247,8 @@ describe('getDonutChartConfig', () => {
         id: 'test-id',
       });
 
-      const label1 = result.labels({ datum: { x: 'Item 1', y: 40 } });
-      const label2 = result.labels({ datum: { x: 'Item 2', y: 60 } });
+      const label1 = result.labels({ datum: { x: 'Item 1', y: 40, name: 'Item 1' } });
+      const label2 = result.labels({ datum: { x: 'Item 2', y: 60, name: 'Item 2' } });
 
       expect(label1).toBe('Item 1: 40.0%');
       expect(label2).toBe('Item 2: 60.0%');