codyml commented on code in PR #21315:
URL: https://github.com/apache/superset/pull/21315#discussion_r972102342
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx:
##########
@@ -25,43 +26,83 @@ import {
t,
validateNonEmpty,
} from '@superset-ui/core';
-import { ExtraControlProps, SharedControlConfig, Dataset } from '../types';
+import {
+ ExtraControlProps,
+ SharedControlConfig,
+ Dataset,
+ Metric,
+} from '../types';
import { DATASET_TIME_COLUMN_OPTION, TIME_FILTER_LABELS } from '../constants';
-import { QUERY_TIME_COLUMN_OPTION, defineSavedMetrics } from '..';
+import {
+ QUERY_TIME_COLUMN_OPTION,
+ defineSavedMetrics,
+ ColumnOption,
+ ColumnMeta,
+ FilterOption,
+} from '..';
import { xAxisControlConfig } from './constants';
-export const dndGroupByControl: SharedControlConfig<'DndColumnSelect'> = {
+type Control = {
+ savedMetrics?: Metric[] | null;
+ default?: unknown;
+};
+
+/*
+ * Note: Previous to the commit that introduced this comment, the shared
controls module
+ * would check feature flags at module execution time and expose a different
control
+ * configuration (component + props) depending on the status of drag-and-drop
feature
+ * flags. This commit combines those configs, merging the required props for
both the
+ * drag-and-drop and non-drag-and-drop components, and renders a wrapper
component that
+ * checks feature flags at component render time to avoid race conditions
between when
+ * feature flags are set and when they're checked.
+ */
+
+export const dndGroupByControl: SharedControlConfig<
+ 'DndColumnSelect' | 'SelectControl',
+ ColumnMeta
+> = {
type: 'DndColumnSelect',
label: t('Dimensions'),
+ multi: true,
+ freeForm: true,
+ clearable: true,
default: [],
+ includeTime: false,
description: t(
- 'One or many columns to group by. High cardinality groupings should
include a series limit ' +
- 'to limit the number of fetched and rendered series.',
+ 'One or many columns to group by. High cardinality groupings should
include a sort by metric ' +
+ 'and series limit to limit the number of fetched and rendered series.',
),
- mapStateToProps(state, { includeTime }) {
+ optionRenderer: (c: ColumnMeta) => <ColumnOption showType column={c} />,
+ valueRenderer: (c: ColumnMeta) => <ColumnOption column={c} />,
+ valueKey: 'column_name',
+ allowAll: true,
+ filterOption: ({ data: opt }: FilterOption<ColumnMeta>, text: string) =>
+ (opt.column_name &&
+ opt.column_name.toLowerCase().includes(text.toLowerCase())) ||
+ (opt.verbose_name &&
+ opt.verbose_name.toLowerCase().includes(text.toLowerCase())) ||
+ false,
+ promptTextCreator: (label: unknown) => label,
+ mapStateToProps(state, controlState) {
const newState: ExtraControlProps = {};
const { datasource } = state;
if (datasource?.columns[0]?.hasOwnProperty('groupby')) {
const options = (datasource as Dataset).columns.filter(c => c.groupby);
- if (includeTime) {
+ if (controlState?.includeTime) {
options.unshift(DATASET_TIME_COLUMN_OPTION);
}
- newState.options = Object.fromEntries(
- options.map(option => [option.column_name, option]),
- );
+ newState.options = options;
newState.savedMetrics = (datasource as Dataset).metrics || [];
} else {
- const options = datasource?.columns;
- if (includeTime) {
- (options as QueryColumn[])?.unshift(QUERY_TIME_COLUMN_OPTION);
+ const options = (datasource?.columns as QueryColumn[]) || [];
+ if (controlState?.includeTime) {
+ options.unshift(QUERY_TIME_COLUMN_OPTION);
}
- newState.options = Object.fromEntries(
- (options as QueryColumn[])?.map(option => [option.name, option]),
- );
- newState.options = datasource?.columns;
+ newState.options = options;
Review Comment:
Yes, I did this because the DnD and non-DnD versions of some shared control
components had different interfaces: the DnD versions expected an object while
the non-DnD versions expected an array. So, to merge the props the two
versions needed to expect the same thing, and the simplest solution seemed to
be to change that so they both expected an array because IIRC it was used as an
array more than it was used as an object in both. Let me know if you think
another solution would have worked better and I can make that change.
--
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]
