Panel: Apply option defaults on panel init and on save model retrieval (#17174)

* Apply panel options defaults on panel init and on save model retrieval

* Remove unnecessary argument, added tests

* Make FieldPropertiesEditor statefull to enable onBlur changes

* Remove unnecessary import

* Post-review updates

Fixes #17154

packages/grafana-ui/src/components/SingleStatShared/FieldPropertiesEditor.tsx

@@ -1,5 +1,5 @@
 // Libraries
-import React, { PureComponent, ChangeEvent } from 'react';
+import React, { ChangeEvent, useState, useCallback } from 'react';
 
 // Components
 import { FormField } from '../FormField/FormField';
@@ -8,7 +8,7 @@ import { UnitPicker } from '../UnitPicker/UnitPicker';
 
 // Types
 import { Field } from '../../types/data';
-import { toNumberString, toIntegerOrUndefined } from '../../utils';
+import { toIntegerOrUndefined } from '../../utils';
 import { SelectOptionItem } from '../Select/Select';
 
 import { VAR_SERIES_NAME, VAR_FIELD_NAME, VAR_CALC, VAR_CELL_PREFIX } from '../../utils/fieldDisplay';
@@ -21,92 +21,108 @@ export interface Props {
   onChange: (value: Partial<Field>, event?: React.SyntheticEvent<HTMLElement>) => void;
 }
 
-export class FieldPropertiesEditor extends PureComponent<Props> {
+export const FieldPropertiesEditor: React.FC<Props> = ({ value, onChange, showMinMax }) => {
-  onTitleChange = (event: ChangeEvent<HTMLInputElement>) =>
+  const { unit, title } = value;
-    this.props.onChange({ ...this.props.value, title: event.target.value });
 
-  // @ts-ignore
+  const [decimals, setDecimals] = useState(
-  onUnitChange = (unit: SelectOptionItem<string>) => this.props.onChange({ ...this.props.value, unit: unit.value });
+    value.decimals !== undefined && value.decimals !== null ? value.decimals.toString() : ''
+  );
+  const [min, setMin] = useState(value.min !== undefined && value.min !== null ? value.min.toString() : '');
+  const [max, setMax] = useState(value.max !== undefined && value.max !== null ? value.max.toString() : '');
 
-  onDecimalChange = (event: ChangeEvent<HTMLInputElement>) => {
+  const onTitleChange = (event: ChangeEvent<HTMLInputElement>) => {
-    this.props.onChange({
+    onChange({ ...value, title: event.target.value });
-      ...this.props.value,
-      decimals: toIntegerOrUndefined(event.target.value),
-    });
   };
 
-  onMinChange = (event: ChangeEvent<HTMLInputElement>) => {
+  const onDecimalChange = useCallback(
-    this.props.onChange({
+    (event: ChangeEvent<HTMLInputElement>) => {
-      ...this.props.value,
+      setDecimals(event.target.value);
-      min: toIntegerOrUndefined(event.target.value),
+    },
-    });
+    [value.decimals, onChange]
+  );
+
+  const onMinChange = useCallback(
+    (event: ChangeEvent<HTMLInputElement>) => {
+      setMin(event.target.value);
+    },
+    [value.min, onChange]
+  );
+
+  const onMaxChange = useCallback(
+    (event: ChangeEvent<HTMLInputElement>) => {
+      setMax(event.target.value);
+    },
+    [value.max, onChange]
+  );
+
+  const onUnitChange = (unit: SelectOptionItem<string>) => {
+    onChange({ ...value, unit: unit.value });
   };
 
-  onMaxChange = (event: ChangeEvent<HTMLInputElement>) => {
+  const commitChanges = useCallback(() => {
-    this.props.onChange({
+    onChange({
-      ...this.props.value,
+      ...value,
-      max: toIntegerOrUndefined(event.target.value),
+      decimals: toIntegerOrUndefined(decimals),
+      min: toIntegerOrUndefined(min),
+      max: toIntegerOrUndefined(max),
     });
-  };
+  }, [min, max, decimals]);
 
-  render() {
+  const titleTooltip = (
-    const { showMinMax } = this.props;
+    <div>
-    const { unit, decimals, min, max } = this.props.value;
+      Template Variables:
+      <br />
+      {'$' + VAR_SERIES_NAME}
+      <br />
+      {'$' + VAR_FIELD_NAME}
+      <br />
+      {'$' + VAR_CELL_PREFIX + '{N}'} / {'$' + VAR_CALC}
+    </div>
+  );
+  return (
+    <>
+      <FormField
+        label="Title"
+        labelWidth={labelWidth}
+        onChange={onTitleChange}
+        value={title}
+        tooltip={titleTooltip}
+        placeholder="Auto"
+      />
 
-    const titleTooltip = (
+      <div className="gf-form">
-      <div>
+        <FormLabel width={labelWidth}>Unit</FormLabel>
-        Template Variables:
+        <UnitPicker defaultValue={unit} onChange={onUnitChange} />
-        <br />
-        {'$' + VAR_SERIES_NAME}
-        <br />
-        {'$' + VAR_FIELD_NAME}
-        <br />
-        {'$' + VAR_CELL_PREFIX + '{N}'} / {'$' + VAR_CALC}
       </div>
-    );
+      {showMinMax && (
-
+        <>
-    return (
+          <FormField
-      <>
+            label="Min"
-        <FormField
+            labelWidth={labelWidth}
-          label="Title"
+            onChange={onMinChange}
-          labelWidth={labelWidth}
+            onBlur={commitChanges}
-          onChange={this.onTitleChange}
+            value={min}
-          value={this.props.value.title}
+            type="number"
-          tooltip={titleTooltip}
+          />
-          placeholder="Auto"
+          <FormField
-        />
+            label="Max"
-
+            labelWidth={labelWidth}
-        <div className="gf-form">
+            onChange={onMaxChange}
-          <FormLabel width={labelWidth}>Unit</FormLabel>
+            onBlur={commitChanges}
-          <UnitPicker defaultValue={unit} onChange={this.onUnitChange} />
+            value={max}
-        </div>
+            type="number"
-        {showMinMax && (
+          />
-          <>
+        </>
-            <FormField
+      )}
-              label="Min"
+      <FormField
-              labelWidth={labelWidth}
+        label="Decimals"
-              onChange={this.onMinChange}
+        labelWidth={labelWidth}
-              value={toNumberString(min)}
+        placeholder="auto"
-              type="number"
+        onChange={onDecimalChange}
-            />
+        onBlur={commitChanges}
-            <FormField
+        value={decimals}
-              label="Max"
+        type="number"
-              labelWidth={labelWidth}
+      />
-              onChange={this.onMaxChange}
+    </>
-              value={toNumberString(max)}
+  );
-              type="number"
+};
-            />
-          </>
-        )}
-        <FormField
-          label="Decimals"
-          labelWidth={labelWidth}
-          placeholder="auto"
-          onChange={this.onDecimalChange}
-          value={toNumberString(decimals)}
-          type="number"
-        />
-      </>
-    );
-  }
-}

public/app/features/dashboard/dashgrid/PanelChrome.tsx

@@ -253,7 +253,7 @@ export class PanelChrome extends PureComponent<Props, State> {
             id={panel.id}
             data={data}
             timeRange={data.request ? data.request.range : this.timeSrv.timeRange()}
-            options={panel.getOptions(plugin.defaults)}
+            options={panel.getOptions()}
             width={width - theme.panelPadding * 2}
             height={innerPanelHeight}
             renderCounter={renderCounter}

public/app/features/dashboard/panel_editor/VisualizationTab.tsx

@@ -53,8 +53,8 @@ export class VisualizationTab extends PureComponent<Props, State> {
   }
 
   getReactPanelOptions = () => {
-    const { panel, plugin } = this.props;
+    const { panel } = this.props;
-    return panel.getOptions(plugin.defaults);
+    return panel.getOptions();
   };
 
   renderPanelOptions() {

public/app/features/dashboard/state/PanelModel.test.ts

@@ -7,45 +7,70 @@ describe('PanelModel', () => {
   describe('when creating new panel model', () => {
     let model;
     let modelJson;
+    let persistedOptionsMock;
+    const defaultOptionsMock = {
+      fieldOptions: {
+        thresholds: [
+          {
+            color: '#F2495C',
+            index: 1,
+            value: 50,
+          },
+          {
+            color: '#73BF69',
+            index: 0,
+            value: null,
+          },
+        ],
+      },
+      showThresholds: true,
+    };
 
     beforeEach(() => {
+      persistedOptionsMock = {
+        fieldOptions: {
+          thresholds: [
+            {
+              color: '#F2495C',
+              index: 1,
+              value: 50,
+            },
+            {
+              color: '#73BF69',
+              index: 0,
+              value: null,
+            },
+          ],
+        },
+      };
+
       modelJson = {
         type: 'table',
         showColumns: true,
         targets: [{ refId: 'A' }, { noRefId: true }],
-        options: {
+        options: persistedOptionsMock,
-          fieldOptions: {
-            thresholds: [
-              {
-                color: '#F2495C',
-                index: 1,
-                value: 50,
-              },
-              {
-                color: '#73BF69',
-                index: 0,
-                value: null,
-              },
-            ],
-          },
-        },
       };
+
       model = new PanelModel(modelJson);
-      model.pluginLoaded(
+      const panelPlugin = getPanelPlugin(
-        getPanelPlugin(
+        {
-          {
+          id: 'table',
-            id: 'table',
+        },
-          },
+        null, // react
-          null, // react
+        TablePanelCtrl // angular
-          TablePanelCtrl // angular
-        )
       );
+      panelPlugin.setDefaults(defaultOptionsMock);
+      model.pluginLoaded(panelPlugin);
     });
 
     it('should apply defaults', () => {
       expect(model.gridPos.h).toBe(3);
     });
 
+    it('should apply option defaults', () => {
+      expect(model.getOptions().showThresholds).toBeTruthy();
+    });
+
     it('should set model props on instance', () => {
       expect(model.showColumns).toBe(true);
     });
@@ -89,11 +114,22 @@ describe('PanelModel', () => {
     });
 
     describe('when changing panel type', () => {
+      const newPanelPluginDefaults = {
+        showThresholdLabels: false,
+      };
+
       beforeEach(() => {
-        model.changePlugin(getPanelPlugin({ id: 'graph' }));
+        const newPlugin = getPanelPlugin({ id: 'graph' });
+        newPlugin.setDefaults(newPanelPluginDefaults);
+        model.changePlugin(newPlugin);
         model.alert = { id: 2 };
       });
 
+      it('should apply next panel option defaults', () => {
+        expect(model.getOptions().showThresholdLabels).toBeFalsy();
+        expect(model.getOptions().showThresholds).toBeUndefined();
+      });
+
       it('should remove table properties but keep core props', () => {
         expect(model.showColumns).toBe(undefined);
       });
@@ -153,19 +189,5 @@ describe('PanelModel', () => {
         expect(panelQueryRunner).toBe(sameQueryRunner);
       });
     });
-
-    describe('get panel options', () => {
-      it('should apply defaults', () => {
-        model.options = { existingProp: 10 };
-        const options = model.getOptions({
-          defaultProp: true,
-          existingProp: 0,
-        });
-
-        expect(options.defaultProp).toBe(true);
-        expect(options.existingProp).toBe(10);
-        expect(model.options).toBe(options);
-      });
-    });
   });
 });

public/app/features/dashboard/state/PanelModel.ts

@@ -157,8 +157,8 @@ export class PanelModel {
     }
   }
 
-  getOptions(panelDefaults: any) {
+  getOptions() {
-    return _.defaultsDeep(this.options || {}, panelDefaults);
+    return this.options;
   }
 
   updateOptions(options: object) {
@@ -179,7 +179,6 @@ export class PanelModel {
 
       model[property] = _.cloneDeep(this[property]);
     }
-
     return model;
   }
 
@@ -247,9 +246,18 @@ export class PanelModel {
     });
   }
 
+  private applyPluginOptionDefaults(plugin: PanelPlugin) {
+    if (plugin.angularConfigCtrl) {
+      return;
+    }
+    this.options = _.defaultsDeep({}, this.options || {}, plugin.defaults);
+  }
+
   pluginLoaded(plugin: PanelPlugin) {
     this.plugin = plugin;
 
+    this.applyPluginOptionDefaults(plugin);
+
     if (plugin.panel && plugin.onPanelMigration) {
       const version = getPluginVersion(plugin);
       if (version !== this.pluginVersion) {
@@ -284,7 +292,7 @@ export class PanelModel {
     // switch
     this.type = pluginId;
     this.plugin = newPlugin;
-
+    this.applyPluginOptionDefaults(newPlugin);
     // Let panel plugins inspect options from previous panel and keep any that it can use
     if (newPlugin.onPanelTypeChanged) {
       this.options = this.options || {};