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.

Sign up for GitHub

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: should be able to remove selection from X-AXIS control #21371

Merged
merged 1 commit into from
Sep 13, 2022
Merged

fix: should be able to remove selection from X-AXIS control #21371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 19 additions & 32 deletions superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/constants.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,48 +19,35 @@
import {
FeatureFlag,
isFeatureEnabled,
QueryFormData,
t,
validateNonEmpty,
} from '@superset-ui/core';
import { ControlPanelState, ControlState } from '../types';

export const xAxisControlConfig = {
label: (state: ControlPanelState) => {
if (
isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) &&
state?.form_data?.orientation === 'horizontal'
) {
return t('Y-axis');
}
const getAxisLabel = (
formData: QueryFormData,
): Record<'label' | 'description', string> =>
formData?.orientation === 'horizontal'
? { label: t('Y-axis'), description: t('Dimension to use on y-axis.') }
: { label: t('X-axis'), description: t('Dimension to use on x-axis.') };

return t('X-axis');
},
default: (
control: ControlState,
controlPanel: Partial<ControlPanelState>,
) => {
// default to the chosen time column if x-axis is unset and the
// GENERIC_CHART_AXES feature flag is enabled
const { value } = control;
if (value) {
return value;
}
const timeColumn = controlPanel?.form_data?.granularity_sqla;
if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && timeColumn) {
return timeColumn;
}
return null;
},
Comment on lines -42 to -53
Copy link
Member

@villebro villebro Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, this logic was added so that charts that were created when the GENERIC_CHART_AXES FF was unset would render correctly when exploring them after the GENERIC_CHART_AXES FF was enabled. Otherwise it would open Explor and clear the chart due to the required x_axis control being unset. So the flow should be something like this:

  • When entering Explore with FF disabled, nothing should happen (x_axis should be null)
  • When entering Explore with the FF enabled and the x_axis value is unset, the value should be set to the temporal column
  • When entering Explore with the FF enabled and the x_axis value is set, it should not be changed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The X-Axis control value should be set instead of setting the default value because if the default value had been set, the X-Axis control wouldn't have chance to remove it(the X-AXIS always sets a default value). Let me find a way to set the X-Axis as the sqla_grainularity if GENERIC_CHART_AXES is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @villebro, I have added a new property for controls, initialValue. It refers to how to initialize a control when loading, this setting avoids the default value always set to the control.

I also tested charts between FF enabled and disabled.

  1. Create a Bar Chart V2 when GENERIC_CHART_AXES is disabled, and then create a dashboard and take the chart.
  2. Open the GENERIC_CHART_AXES
  3. the Chart on the Explore page and Dashboard same as the disabled FF.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @villebro, I have added a new property for controls, initialValue. It refers to how to initialize a control when loading, this setting avoids the default value always set to the control.

This is a really useful addition 👍 I'm sure this will be needed in the future at some point.

export const xAxisControlConfig = {
label: (state: ControlPanelState) => getAxisLabel(state?.form_data).label,
multi: false,
description: (state: ControlPanelState) => {
description: (state: ControlPanelState) =>
getAxisLabel(state?.form_data).description,
validators: [validateNonEmpty],
initialValue: (control: ControlState, state: ControlPanelState) => {
if (
isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) &&
state?.form_data?.orientation === 'horizontal'
state?.form_data?.granularity_sqla &&
!state.form_data?.x_axis &&
!control?.value
) {
return t('Dimension to use on y-axis.');
return state.form_data.granularity_sqla;
}

return t('Dimension to use on x-axis.');
return undefined;
},
validators: [validateNonEmpty],
default: undefined,
};
1 change: 1 addition & 0 deletions superset-frontend/packages/superset-ui-chart-controls/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ export interface BaseControlConfig<
chartState?: AnyDict,
) => ReactNode);
default?: V;
initialValue?: V;
renderTrigger?: boolean;
validators?: ControlValueValidator<T, O, V>[];
warning?: ReactNode;
Expand Down
15 changes: 15 additions & 0 deletions superset-frontend/src/explore/controlUtils/getControlState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,21 @@ export function applyMapStateToPropsToControl<T = ControlType>(
// `mapStateToProps` may also provide a value
value = value || state.value;
}

// InitialValue is used for setting value for the control,
// this value is not recalculated. The default value will override it.
if (typeof state.initialValue === 'function') {
state.initialValue = state.initialValue(state, controlPanelState);
// if default is still a function, discard
if (typeof state.initialValue === 'function') {
delete state.initialValue;
}
}
if (state.initialValue) {
value = state.initialValue;
delete state.initialValue;
}

// If default is a function, evaluate it
if (typeof state.default === 'function') {
state.default = state.default(state, controlPanelState);
Expand Down