villebro commented on code in PR #21371:
URL: https://github.com/apache/superset/pull/21371#discussion_r965783606
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/constants.tsx:
##########
@@ -22,45 +22,17 @@ import {
t,
validateNonEmpty,
} from '@superset-ui/core';
-import { ControlPanelState, ControlState } from '../types';
+import { ControlPanelState } 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 = (state: ControlPanelState): [string, string] =>
Review Comment:
nit: For clarity, could we make the return type `{ label: string,
description: string }` so we then later do `getAxisLabel(state).label` instead
of `getAxisLabel(state)[0]`?
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/constants.tsx:
##########
@@ -22,45 +22,17 @@ import {
t,
validateNonEmpty,
} from '@superset-ui/core';
-import { ControlPanelState, ControlState } from '../types';
+import { ControlPanelState } 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 = (state: ControlPanelState): [string, string] =>
+ isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) &&
+ state?.form_data?.orientation === 'horizontal'
+ ? [t('Y-axis'), t('Dimension to use on y-axis.')]
+ : [t('X-axis'), 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;
- },
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
