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

chore: Deprecate DND feature flags #23981

Merged
merged 4 commits into from
May 10, 2023
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
3 changes: 1 addition & 2 deletions RESOURCES/FEATURE_FLAGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ These features flags are **safe for production**. They have been tested and will
- DRUID_JOINS
- EMBEDDABLE_CHARTS
- EMBEDDED_SUPERSET
- ENABLE_DND_WITH_CLICK_UX
- ENABLE_EXPLORE_DRAG_AND_DROP
- ENABLE_TEMPLATE_PROCESSING
- ESCAPE_MARKDOWN_HTML
- LISTVIEWS_DEFAULT_CARD_VIEW
Expand All @@ -96,6 +94,7 @@ These features flags currently default to True and **will be removed in a future
- DASHBOARD_NATIVE_FILTERS
- DASHBOARD_NATIVE_FILTERS_SET
- DISABLE_DATASET_SOURCE_EDIT
- ENABLE_EXPLORE_DRAG_AND_DROP
- ENABLE_EXPLORE_JSON_CSRF_PROTECTION
- GENERIC_CHART_AXES
- REMOVE_SLICE_LEVEL_LABEL_COLORS
Original file line number Diff line number Diff line change
Expand Up @@ -244,8 +244,7 @@ export const dndGranularitySqlaControl: typeof dndSeriesControl = {
default: (c: Control) => c.default,
clearable: false,
canDelete: false,
ghostButtonText: t('Drop temporal column here'),
clickEnabledGhostButtonText: t('Drop a temporal column here or click'),
ghostButtonText: t('Drop a temporal column here or click'),
optionRenderer: (c: ColumnMeta) => <ColumnOption showType column={c} />,
valueRenderer: (c: ColumnMeta) => <ColumnOption column={c} />,
valueKey: 'column_name',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ export enum FeatureFlag {
EMBEDDABLE_CHARTS = 'EMBEDDABLE_CHARTS',
EMBEDDED_SUPERSET = 'EMBEDDED_SUPERSET',
ENABLE_ADVANCED_DATA_TYPES = 'ENABLE_ADVANCED_DATA_TYPES',
ENABLE_DND_WITH_CLICK_UX = 'ENABLE_DND_WITH_CLICK_UX',
ENABLE_EXPLORE_DRAG_AND_DROP = 'ENABLE_EXPLORE_DRAG_AND_DROP',
ENABLE_JAVASCRIPT_CONTROLS = 'ENABLE_JAVASCRIPT_CONTROLS',
ENABLE_TEMPLATE_PROCESSING = 'ENABLE_TEMPLATE_PROCESSING',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ test('renders with default props', async () => {
useDnd: true,
useRedux: true,
});
expect(await screen.findByText('Drop columns here')).toBeInTheDocument();
expect(
await screen.findByText('Drop columns here or click'),
).toBeInTheDocument();
});

test('renders with value', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
import React, { useCallback, useMemo, useState } from 'react';
import {
AdhocColumn,
FeatureFlag,
isFeatureEnabled,
tn,
QueryFormColumn,
t,
Expand Down Expand Up @@ -54,7 +52,6 @@ function DndColumnSelect(props: DndColumnSelectProps) {
onChange,
canDelete = true,
ghostButtonText,
clickEnabledGhostButtonText,
name,
label,
isTemporal,
Expand Down Expand Up @@ -108,19 +105,14 @@ function DndColumnSelect(props: DndColumnSelectProps) {
[onChange, optionSelector],
);

const clickEnabled = useMemo(
() => isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX),
[],
);

const valuesRenderer = useCallback(
() =>
optionSelector.values.map((column, idx) => {
const datasourceWarningMessage =
isAdhocColumn(column) && column.datasourceWarning
? t('This column might be incompatible with current dataset')
: undefined;
return clickEnabled ? (
return (
<ColumnSelectPopoverTrigger
key={idx}
columns={options}
Expand All @@ -147,22 +139,10 @@ function DndColumnSelect(props: DndColumnSelectProps) {
withCaret
/>
</ColumnSelectPopoverTrigger>
) : (
<OptionWrapper
key={idx}
index={idx}
clickClose={onClickClose}
onShiftOptions={onShiftOptions}
type={`${DndItemType.ColumnOption}_${name}_${label}`}
canDelete={canDelete}
column={column}
datasourceWarningMessage={datasourceWarningMessage}
/>
);
}),
[
canDelete,
clickEnabled,
isTemporal,
label,
name,
Expand Down Expand Up @@ -198,24 +178,16 @@ function DndColumnSelect(props: DndColumnSelectProps) {
togglePopover(true);
}, [togglePopover]);

const labelGhostButtonText = useMemo(() => {
if (clickEnabled) {
return (
clickEnabledGhostButtonText ??
ghostButtonText ??
tn(
'Drop a column here or click',
'Drop columns here or click',
multi ? 2 : 1,
)
);
}

return (
const labelGhostButtonText = useMemo(
() =>
ghostButtonText ??
tn('Drop column here', 'Drop columns here', multi ? 2 : 1)
);
}, [clickEnabled, clickEnabledGhostButtonText, ghostButtonText, multi]);
tn(
'Drop a column here or click',
'Drop columns here or click',
multi ? 2 : 1,
),
[ghostButtonText, multi],
);

return (
<div>
Expand All @@ -226,7 +198,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
accept={DndItemType.Column}
displayGhostButton={multi || optionSelector.values.length === 0}
ghostButtonText={labelGhostButtonText}
onClickGhostButton={clickEnabled ? openPopover : undefined}
onClickGhostButton={openPopover}
{...props}
/>
<ColumnSelectPopoverTrigger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ function setup({
test('renders with default props', async () => {
render(setup(), { useDnd: true });
expect(
await screen.findByText('Drop columns or metrics here'),
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});

Expand Down Expand Up @@ -122,7 +122,7 @@ test('renders options with saved metric', async () => {
},
);
expect(
await screen.findByText('Drop columns or metrics here'),
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});

Expand All @@ -143,7 +143,7 @@ test('renders options with column', async () => {
},
);
expect(
await screen.findByText('Drop columns or metrics here'),
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});

Expand All @@ -165,6 +165,6 @@ test('renders options with adhoc metric', async () => {
},
);
expect(
await screen.findByText('Drop columns or metrics here'),
await screen.findByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@
*/
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import {
FeatureFlag,
hasGenericChartAxes,
isFeatureEnabled,
logging,
Metric,
QueryFormData,
Expand Down Expand Up @@ -397,23 +395,15 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
[controlName, togglePopover],
);

const ghostButtonText = isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
? t('Drop columns/metrics here or click')
: t('Drop columns or metrics here');

return (
<>
<DndSelectLabel
onDrop={handleDrop}
canDrop={canDrop}
valuesRenderer={valuesRenderer}
accept={DND_ACCEPTED_TYPES}
ghostButtonText={ghostButtonText}
onClickGhostButton={
isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
? handleClickGhostButton
: undefined
}
ghostButtonText={t('Drop columns/metrics here or click')}
onClickGhostButton={handleClickGhostButton}
{...props}
/>
<AdhocFilterPopoverTrigger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,16 @@ afterAll(() => {

test('renders with default props', () => {
render(<DndMetricSelect {...defaultProps} />, { useDnd: true });
expect(screen.getByText('Drop column or metric here')).toBeInTheDocument();
expect(
screen.getByText('Drop a column/metric here or click'),
).toBeInTheDocument();
});

test('renders with default props and multi = true', () => {
render(<DndMetricSelect {...defaultProps} multi />, { useDnd: true });
expect(screen.getByText('Drop columns or metrics here')).toBeInTheDocument();
expect(
screen.getByText('Drop columns/metrics here or click'),
).toBeInTheDocument();
});

test('render selected metrics correctly', () => {
Expand Down Expand Up @@ -159,7 +163,7 @@ test('remove selected custom metric when metric gets removed from dataset for si

expect(screen.getByText('Metric B')).toBeVisible();
expect(
screen.queryByText('Drop column or metric here'),
screen.queryByText('Drop a column/metric here or click'),
).not.toBeInTheDocument();

const newPropsWithRemovedMetric = {
Expand All @@ -182,7 +186,7 @@ test('remove selected custom metric when metric gets removed from dataset for si
);

expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
expect(screen.getByText('Drop column or metric here')).toBeVisible();
expect(screen.getByText('Drop a column/metric here or click')).toBeVisible();
});

test('remove selected adhoc metric when column gets removed from dataset', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,8 @@
import React, { useCallback, useEffect, useMemo, useState } from 'react';
import {
ensureIsArray,
FeatureFlag,
GenericDataType,
isAdhocMetricSimple,
isFeatureEnabled,
isSavedMetric,
Metric,
QueryFormMetric,
Expand Down Expand Up @@ -329,17 +327,11 @@ const DndMetricSelect = (props: any) => {
return new AdhocMetric({});
}, [droppedItem]);

const ghostButtonText = isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
? tn(
'Drop a column/metric here or click',
'Drop columns/metrics here or click',
multi ? 2 : 1,
)
: tn(
'Drop column or metric here',
'Drop columns or metrics here',
multi ? 2 : 1,
);
const ghostButtonText = tn(
'Drop a column/metric here or click',
'Drop columns/metrics here or click',
multi ? 2 : 1,
);

return (
<div className="metrics-select">
Expand All @@ -350,11 +342,7 @@ const DndMetricSelect = (props: any) => {
accept={DND_ACCEPTED_TYPES}
ghostButtonText={ghostButtonText}
displayGhostButton={multi || value.length === 0}
onClickGhostButton={
isFeatureEnabled(FeatureFlag.ENABLE_DND_WITH_CLICK_UX)
? handleClickGhostButton
: undefined
}
onClickGhostButton={handleClickGhostButton}
{...props}
/>
<AdhocMetricPopoverTrigger
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
import userEvent from '@testing-library/user-event';
import { render, screen } from 'spec/helpers/testing-library';
import { DndItemType } from 'src/explore/components/DndItemType';
import DndSelectLabel, {
Expand All @@ -29,27 +30,35 @@ const defaultProps: DndSelectLabelProps = {
onDrop: jest.fn(),
canDrop: () => false,
valuesRenderer: () => <span />,
ghostButtonText: 'Drop columns here or click',
onClickGhostButton: jest.fn(),
};

test('renders with default props', async () => {
test('renders with default props', () => {
render(<DndSelectLabel {...defaultProps} />, { useDnd: true });
expect(await screen.findByText('Drop columns here')).toBeInTheDocument();
expect(screen.getByText('Drop columns here or click')).toBeInTheDocument();
});

test('renders ghost button when empty', async () => {
test('renders ghost button when empty', () => {
const ghostButtonText = 'Ghost button text';
render(
<DndSelectLabel {...defaultProps} ghostButtonText={ghostButtonText} />,
{ useDnd: true },
);
expect(await screen.findByText(ghostButtonText)).toBeInTheDocument();
expect(screen.getByText(ghostButtonText)).toBeInTheDocument();
});

test('renders values', async () => {
test('renders values', () => {
const values = 'Values';
const valuesRenderer = () => <span>{values}</span>;
render(<DndSelectLabel {...defaultProps} valuesRenderer={valuesRenderer} />, {
useDnd: true,
});
expect(await screen.findByText(values)).toBeInTheDocument();
expect(screen.getByText(values)).toBeInTheDocument();
});

test('Handles ghost button click', () => {
render(<DndSelectLabel {...defaultProps} />, { useDnd: true });
userEvent.click(screen.getByText('Drop columns here or click'));
expect(defaultProps.onClickGhostButton).toHaveBeenCalled();
});
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@ import { DndItemType } from '../../DndItemType';
export type DndSelectLabelProps = {
name: string;
accept: DndItemType | DndItemType[];
ghostButtonText?: string;
ghostButtonText: string;
onDrop: (item: DatasourcePanelDndItem) => void;
canDrop: (item: DatasourcePanelDndItem) => boolean;
canDropValue?: (value: DndItemValue) => boolean;
onDropValue?: (value: DndItemValue) => void;
valuesRenderer: () => ReactNode;
displayGhostButton?: boolean;
onClickGhostButton?: () => void;
onClickGhostButton: () => void;
};

export default function DndSelectLabel({
Expand Down Expand Up @@ -80,7 +80,7 @@ export default function DndSelectLabel({
onClick={props.onClickGhostButton}
>
<Icons.PlusSmall iconColor={theme.colors.grayscale.light1} />
{t(props.ghostButtonText || 'Drop columns here')}
{t(props.ghostButtonText)}
</AddControlLabel>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ export type DndControlProps<ValueType extends JsonValue> =
multi?: boolean;
canDelete?: boolean;
ghostButtonText?: string;
clickEnabledGhostButtonText?: string;
onChange: (value: ValueType | ValueType[] | null | undefined) => void;
};

Expand Down
3 changes: 1 addition & 2 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -458,9 +458,8 @@ class D3Format(TypedDict, total=False):
# Enables Alerts and reports new implementation
"ALERT_REPORTS": False,
"DASHBOARD_RBAC": False,
"ENABLE_EXPLORE_DRAG_AND_DROP": True,
"ENABLE_EXPLORE_DRAG_AND_DROP": True, # deprecated
"ENABLE_ADVANCED_DATA_TYPES": False,
"ENABLE_DND_WITH_CLICK_UX": True,
# Enabling ALERTS_ATTACH_REPORTS, the system sends email and slack message
# with screenshot and link
# Disables ALERTS_ATTACH_REPORTS, the system DOES NOT generate screenshot
Expand Down