fix: Pivot Table Conditional Formatting Doesn't Show All Options (#19071

)

* fix: Pivot Table Conditional Formatting Doesn't Show All Options

* PR comments

* PR comments

superset-frontend/packages/superset-ui-chart-controls/src/types.ts

@@ -171,6 +171,8 @@ export type TabOverride = 'data' | 'customize' | boolean;
  *    bubbled up to the control header, section header and query panel header.
  * - warning: text shown as a tooltip on a warning icon in the control's header
  * - error: text shown as a tooltip on a error icon in the control's header
+ * - shouldMapStateToProps: a function that receives the previous and current app state
+ *   and determines if the control needs to recalculate it's props based on the new state.
  * - mapStateToProps: a function that receives the App's state and return an object of k/v
  *    to overwrite configuration at runtime. This is useful to alter a component based on
  *    anything external to it, like another control's value. For instance it's possible to
@@ -198,6 +200,13 @@ export interface BaseControlConfig<
   /**
    * Add additional props to chart control.
    */
+  shouldMapStateToProps?: (
+    prevState: ControlPanelState,
+    state: ControlPanelState,
+    controlState: ControlState,
+    // TODO: add strict `chartState` typing (see superset-frontend/src/explore/types)
+    chartState?: AnyDict,
+  ) => boolean;
   mapStateToProps?: (
     state: ControlPanelState,
     controlState: ControlState,

superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx

@@ -170,7 +170,10 @@ const config: ControlPanelConfig = {
               configFormLayout: {
                 [GenericDataType.NUMERIC]: [[radarMetricMaxValue]],
               },
-              mapStateToProps(explore, control, chart) {
+              shouldMapStateToProps() {
+                return true;
+              },
+              mapStateToProps(explore, _, chart) {
                 const values =
                   (explore?.controls?.metrics?.value as QueryFormMetric[]) ??
                   [];

superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx

@@ -66,6 +66,7 @@ const config: ControlPanelConfig = {
             config: {
               ...sharedControls.metrics,
               validators: [validateNonEmpty],
+              rerender: ['conditional_formatting'],
             },
           },
         ],

superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx

@@ -457,7 +457,10 @@ const config: ControlPanelConfig = {
               label: t('Customize columns'),
               description: t('Further customize how to display each column'),
               renderTrigger: true,
-              mapStateToProps(explore, control, chart) {
+              shouldMapStateToProps() {
+                return true;
+              },
+              mapStateToProps(explore, _, chart) {
                 return {
                   queryResponse: chart?.queriesResponse?.[0] as
                     | ChartDataResponseResult
@@ -478,7 +481,10 @@ const config: ControlPanelConfig = {
               description: t(
                 'Apply conditional color formatting to numeric columns',
               ),
-              mapStateToProps(explore, control, chart) {
+              shouldMapStateToProps() {
+                return true;
+              },
+              mapStateToProps(explore, _, chart) {
                 const verboseMap = explore?.datasource?.verbose_map ?? {};
                 const { colnames, coltypes } =
                   chart?.queriesResponse?.[0] ?? {};

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

@@ -187,6 +187,7 @@ function getState(
 export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
   const pluginContext = useContext(PluginContext);
 
+  const prevState = usePrevious(props.exploreState);
   const prevDatasource = usePrevious(props.exploreState.datasource);
 
   const [showDatasourceAlert, setShowDatasourceAlert] = useState(false);
@@ -221,7 +222,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
         props.datasource_type,
       ),
     [
-      props.form_data.datasource,
+      props.exploreState.datasource,
       props.form_data.viz_type,
       props.datasource_type,
     ],
@@ -245,6 +246,22 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
     setShowDatasourceAlert(false);
   }, []);
 
+  const shouldRecalculateControlState = ({
+    name,
+    config,
+  }: CustomControlItem): boolean => {
+    const { controls, chart, exploreState } = props;
+
+    return Boolean(
+      config.shouldMapStateToProps?.(
+        prevState || exploreState,
+        exploreState,
+        controls[name],
+        chart,
+      ),
+    );
+  };
+
   const renderControl = ({ name, config }: CustomControlItem) => {
     const { controls, chart, exploreState } = props;
     const { visibility } = config;
@@ -255,11 +272,8 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
     const controlData = {
       ...config,
       ...controls[name],
-      // if `mapStateToProps` accept three arguments, it means it needs chart
-      // state, too. Since it's may be expensive to run mapStateToProps for every
-      // re-render, we only run this when the chart plugin explicitly ask for this.
-      ...(config.mapStateToProps?.length === 3
-        ? config.mapStateToProps(exploreState, controls[name], chart)
+      ...(shouldRecalculateControlState({ name, config })
+        ? config?.mapStateToProps?.(exploreState, controls[name], chart)
         : // for other controls, `mapStateToProps` is already run in
           // controlUtils/getControlState.ts
           undefined),