From 0e0beceac173f765d8f9a0887732029b78603f6d Mon Sep 17 00:00:00 2001 From: Diego Medina Date: Wed, 9 Mar 2022 17:04:04 -0500 Subject: [PATCH] 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-ui-chart-controls/src/types.ts | 9 +++++++ .../src/Radar/controlPanel.tsx | 5 +++- .../src/plugin/controlPanel.tsx | 1 + .../plugin-chart-table/src/controlPanel.tsx | 10 +++++-- .../components/ControlPanelsContainer.tsx | 26 ++++++++++++++----- 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 9e4e204866..4eec6e9c0b 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/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, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx index 5819b8a70b..0f8e390802 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/controlPanel.tsx +++ b/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[]) ?? []; diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx index b546c08cc6..99efb8bbcc 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx +++ b/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'], }, }, ], diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx index 379568384f..361f29b635 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/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] ?? {}; diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index ed98570881..650e5f00c0 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/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),