Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[]) ??
[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ const config: ControlPanelConfig = {
config: {
...sharedControls.metrics,
validators: [validateNonEmpty],
rerender: ['conditional_formatting'],
},
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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] ?? {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
],
Expand All @@ -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;
Expand All @@ -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),
Expand Down