Skip to content

Commit

Permalink
chore: Rename and reposition native filter modal fields (#18558)
Browse files Browse the repository at this point in the history
* chore: Rename and reposition native filter modal fields

* Fixes collapse initial state

* Changes the button to sentence case
michael-s-molina authored Feb 8, 2022
1 parent 125be78 commit fdf57cc
Showing 15 changed files with 395 additions and 430 deletions.
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ import qs from 'querystring';
import { dashboardView, nativeFilters } from 'cypress/support/directories';
import { testItems } from './dashboard.helper';
import { DASHBOARD_LIST } from '../dashboard_list/dashboard_list.helper';
import { CHART_LIST } from '../chart_list/chart_list.helper';

const getTestTitle = (
test: Mocha.Suite = (Cypress as any).mocha.getRunner().suite.ctx.test,
@@ -43,7 +44,8 @@ describe('Nativefilters Sanity test', () => {
).then(xhr => {
const dashboards = xhr.body.result;
const worldBankDashboard = dashboards.find(
d => d.dashboard_title === "World Bank's Data",
(d: { dashboard_title: string }) =>
d.dashboard_title === "World Bank's Data",
);
cy.visit(worldBankDashboard.url);
});
@@ -65,7 +67,8 @@ describe('Nativefilters Sanity test', () => {
).then(xhr => {
const dashboards = xhr.body.result;
const testDashboard = dashboards.find(
d => d.dashboard_title === testItems.dashboard,
(d: { dashboard_title: string }) =>
d.dashboard_title === testItems.dashboard,
);
cy.visit(testDashboard.url);
});
@@ -107,13 +110,14 @@ describe('Nativefilters Sanity test', () => {
cy.get(nativeFilters.createFilterButton).should('be.visible').click();
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.filterName)
.click()
.click({ force: true })
.type('Country name');
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.datasetName)
.click()
.type('wb_health_population{enter}');

.click({ force: true })
.within(() =>
cy.get('input').type('wb_health_population{enter}', { force: true }),
);
// Add following step to avoid flaky enter value in line 177
cy.get(nativeFilters.filtersPanel.inputDropdown)
.should('be.visible', { timeout: 20000 })
@@ -163,7 +167,7 @@ describe('Nativefilters Sanity test', () => {
cy.get(nativeFilters.createFilterButton).click({ force: true });
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.filterName)
.click()
.click({ force: true })
.type('suffix');
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.datasetName)
@@ -218,12 +222,14 @@ describe('Nativefilters Sanity test', () => {
cy.get(nativeFilters.modal.container).should('be.visible');
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.filterName)
.click()
.click({ force: true })
.type('Country name');
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.datasetName)
.click()
.type('wb_health_population{enter}');
.click({ force: true })
.within(() =>
cy.get('input').type('wb_health_population{enter}', { force: true }),
);

cy.get('.loading inline-centered css-101mkpk').should('not.exist');
// hack for unclickable country_name
@@ -255,53 +261,42 @@ describe('Nativefilters Sanity test', () => {
cy.get(nativeFilters.filterFromDashboardView.expand).click({ force: true });
cy.get(nativeFilters.createFilterButton).should('be.visible').click();
cy.get(nativeFilters.modal.container).should('be.visible');
cy.get(nativeFilters.filterConfigurationSections.collapseExpandButton)
.last()
.click();
[
'Filter has default value',
'Multiple select',
'Required',
'Can select multiple values',
'Filter value is required',
'Filter is hierarchical',
'Default to first item',
'Select first filter value by default',
'Inverse selection',
'Search all filter options',
'Dynamically search all filter values',
'Pre-filter available values',
'Sort filter values',
].forEach(el => {
cy.contains(el);
});
cy.get(nativeFilters.filterConfigurationSections.checkedCheckbox).contains(
'Multiple select',
'Can select multiple values',
);
cy.get(nativeFilters.filterConfigurationSections.infoTooltip)
.eq(0)
.trigger('mouseover');
cy.contains('Allow selecting multiple values');
.trigger('mouseover', { force: true });
cy.contains('User must select a value before applying the filter');

cy.get(nativeFilters.filterConfigurationSections.infoTooltip)
.eq(1)
.trigger('mouseover');
cy.contains('User must select a value before applying the filter');
.trigger('mouseover', { force: true });
cy.contains('When using this option, default value can’t be set');

cy.get(nativeFilters.filterConfigurationSections.infoTooltip)
.eq(2)
.trigger('mouseover');
.trigger('mouseover', { force: true });
cy.contains(
'Select first item by default (when using this option, default value can’t be set)',
'By default, each filter loads at most 1000 choices at the initial page load. Check this box if you have more than 1000 filter values and want to enable dynamically searching that loads filter values as users type (may add stress to your database).',
);

cy.get(nativeFilters.filterConfigurationSections.infoTooltip)
.eq(3)
.trigger('mouseover');
.trigger('mouseover', { force: true });
cy.contains('Exclude selected values');

cy.get(nativeFilters.filterConfigurationSections.infoTooltip)
.eq(4)
.trigger('mouseover');
cy.contains(
'By default, each filter loads at most 1000 choices at the initial page load. Check this box if you have more than 1000 filter values and want to enable dynamically searching that loads filter values as users type (may add stress to your database).',
);
});
it("User can check 'Filter has default value'", () => {
cy.get(nativeFilters.filterFromDashboardView.expand).click({ force: true });
@@ -312,11 +307,13 @@ describe('Nativefilters Sanity test', () => {

cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.datasetName)
.click()
.type('wb_health_population{enter}');
.click({ force: true })
.within(() =>
cy.get('input').type('wb_health_population{enter}', { force: true }),
);
cy.get(nativeFilters.modal.container)
.find(nativeFilters.filtersPanel.filterName)
.click()
.click({ force: true })
.type('country_name');
// hack for unclickable datetime
cy.wait(5000);
Original file line number Diff line number Diff line change
@@ -100,7 +100,7 @@ test('remove filter', async () => {
test('add filter', async () => {
defaultRender();
// First trash icon
const addButton = screen.getByText('Add')!;
const addButton = screen.getByText('Add filters and dividers')!;
fireEvent.mouseOver(addButton);
const addFilterButton = await screen.findByText('Filter');

@@ -118,7 +118,7 @@ test('add filter', async () => {

test('add divider', async () => {
defaultRender();
const addButton = screen.getByText('Add')!;
const addButton = screen.getByText('Add filters and dividers')!;
fireEvent.mouseOver(addButton);
const addFilterButton = await screen.findByText('Divider');
await act(async () => {
Original file line number Diff line number Diff line change
@@ -36,27 +36,13 @@ interface Props {
erroredFilters: string[];
}

const StyledPlusButton = styled.div`
color: ${({ theme }) => theme.colors.primary.dark1};
`;

const StyledHeader = styled.div`
${({ theme }) => `
color: ${theme.colors.grayscale.dark1};
font-size: ${theme.typography.sizes.l}px;
padding-top: ${theme.gridUnit * 4}px;
padding-right: ${theme.gridUnit * 4}px;
padding-left: ${theme.gridUnit * 4}px;
padding-bottom: ${theme.gridUnit * 2}px;
`}
`;

const StyledAddBox = styled.div`
${({ theme }) => `
cursor: pointer;
margin: ${theme.gridUnit * 4}px;
color: ${theme.colors.primary.base};
&:hover {
color: ${theme.colors.primary.base};
color: ${theme.colors.primary.dark1};
}
`}
`;
@@ -104,7 +90,12 @@ const FilterTitlePane: React.FC<Props> = ({
);
return (
<TabsContainer>
<StyledHeader>Filters</StyledHeader>
<Dropdown overlay={menu} arrow placement="topLeft" trigger={['hover']}>
<StyledAddBox>
<div data-test="new-dropdown-icon" className="fa fa-plus" />{' '}
<span>{t('Add filters and dividers')}</span>
</StyledAddBox>
</Dropdown>
<div
css={{
height: '100%',
@@ -124,15 +115,6 @@ const FilterTitlePane: React.FC<Props> = ({
restoreFilter={restoreFilter}
/>
</div>
<Dropdown overlay={menu} arrow placement="topLeft" trigger={['hover']}>
<StyledAddBox>
<StyledPlusButton
data-test="new-dropdown-icon"
className="fa fa-plus"
/>{' '}
<span>{t('Add')}</span>
</StyledAddBox>
</Dropdown>
</TabsContainer>
);
};
Original file line number Diff line number Diff line change
@@ -43,7 +43,7 @@ describe('FilterScope', () => {
save,
removedFilters: {},
handleActiveFilterPanelChange: jest.fn(),
activeFilterPanelKeys: `DefaultFilterId-${FilterPanels.basic.key}`,
activeFilterPanelKeys: `DefaultFilterId-${FilterPanels.configuration.key}`,
isActive: true,
};

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -87,10 +87,12 @@ export const useDefaultValue = (
let tooltip = '';
if (defaultToFirstItem) {
tooltip = t(
'Default value set automatically when "Default to first item" is checked',
'Default value set automatically when "Select first filter value by default" is checked',
);
} else if (isRequired) {
tooltip = t('Default value must be set when "Required" is checked');
tooltip = t(
'Default value must be set when "Filter value is required" is checked',
);
} else if (hasDefaultValue) {
tooltip = t(
'Default value must be set when "Filter has default value" is checked',
Original file line number Diff line number Diff line change
@@ -120,14 +120,14 @@ const NUMERICAL_RANGE_REGEX = /^numerical range$/i;
const TIME_RANGE_REGEX = /^time range$/i;
const TIME_COLUMN_REGEX = /^time column$/i;
const TIME_GRAIN_REGEX = /^time grain$/i;
const ADVANCED_REGEX = /^advanced$/i;
const FILTER_SETTINGS_REGEX = /^filter settings$/i;
const DEFAULT_VALUE_REGEX = /^filter has default value$/i;
const MULTIPLE_REGEX = /^multiple select$/i;
const REQUIRED_REGEX = /^required$/i;
const MULTIPLE_REGEX = /^can select multiple values$/i;
const REQUIRED_REGEX = /^filter value is required$/i;
const HIERARCHICAL_REGEX = /^filter is hierarchical$/i;
const FIRST_ITEM_REGEX = /^default to first item$/i;
const FIRST_VALUE_REGEX = /^select first filter value by default$/i;
const INVERSE_SELECTION_REGEX = /^inverse selection$/i;
const SEARCH_ALL_REGEX = /^search all filter options$/i;
const SEARCH_ALL_REGEX = /^dynamically search all filter values$/i;
const PRE_FILTER_REGEX = /^pre-filter available values$/i;
const SORT_REGEX = /^sort filter values$/i;
const SAVE_REGEX = /^save$/i;
@@ -169,7 +169,7 @@ function queryCheckbox(name: RegExp) {
test('renders a value filter type', () => {
defaultRender();

userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
@@ -179,7 +179,7 @@ test('renders a value filter type', () => {
expect(getCheckbox(DEFAULT_VALUE_REGEX)).not.toBeChecked();
expect(getCheckbox(REQUIRED_REGEX)).not.toBeChecked();
expect(getCheckbox(HIERARCHICAL_REGEX)).not.toBeChecked();
expect(getCheckbox(FIRST_ITEM_REGEX)).not.toBeChecked();
expect(getCheckbox(FIRST_VALUE_REGEX)).not.toBeChecked();
expect(getCheckbox(INVERSE_SELECTION_REGEX)).not.toBeChecked();
expect(getCheckbox(SEARCH_ALL_REGEX)).not.toBeChecked();
expect(getCheckbox(PRE_FILTER_REGEX)).not.toBeChecked();
@@ -195,7 +195,7 @@ test('renders a numerical range filter type', async () => {

await waitFor(() => userEvent.click(screen.getByText(NUMERICAL_RANGE_REGEX)));

userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));

expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument();
expect(screen.getByText(FILTER_NAME_REGEX)).toBeInTheDocument();
@@ -208,7 +208,7 @@ test('renders a numerical range filter type', async () => {

expect(queryCheckbox(MULTIPLE_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(HIERARCHICAL_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(FIRST_ITEM_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(FIRST_VALUE_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(INVERSE_SELECTION_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(SEARCH_ALL_REGEX)).not.toBeInTheDocument();
expect(queryCheckbox(SORT_REGEX)).not.toBeInTheDocument();
@@ -227,8 +227,6 @@ test('renders a time range filter type', async () => {
expect(screen.queryByText(COLUMN_REGEX)).not.toBeInTheDocument();

expect(getCheckbox(DEFAULT_VALUE_REGEX)).not.toBeChecked();

expect(screen.queryByText(ADVANCED_REGEX)).not.toBeInTheDocument();
});

test('renders a time column filter type', async () => {
@@ -244,8 +242,6 @@ test('renders a time column filter type', async () => {
expect(screen.queryByText(COLUMN_REGEX)).not.toBeInTheDocument();

expect(getCheckbox(DEFAULT_VALUE_REGEX)).not.toBeChecked();

expect(screen.queryByText(ADVANCED_REGEX)).not.toBeInTheDocument();
});

test('renders a time grain filter type', async () => {
@@ -261,8 +257,6 @@ test('renders a time grain filter type', async () => {
expect(screen.queryByText(COLUMN_REGEX)).not.toBeInTheDocument();

expect(getCheckbox(DEFAULT_VALUE_REGEX)).not.toBeChecked();

expect(screen.queryByText(ADVANCED_REGEX)).not.toBeInTheDocument();
});

test('render time filter types as disabled if there are no temporal columns in the dataset', async () => {
@@ -310,14 +304,14 @@ test.skip('validates the default value', async () => {

test('validates the hierarchical value', async () => {
defaultRender();
userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
userEvent.click(getCheckbox(HIERARCHICAL_REGEX));
expect(await screen.findByText(PARENT_REQUIRED_REGEX)).toBeInTheDocument();
});

test('validates the pre-filter value', async () => {
defaultRender();
userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
userEvent.click(getCheckbox(PRE_FILTER_REGEX));
expect(
await screen.findByText(PRE_FILTER_REQUIRED_REGEX),
@@ -332,7 +326,7 @@ test.skip("doesn't render time range pre-filter if there are no temporal columns
expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument();
userEvent.click(screen.getByText('birth_names'));
});
userEvent.click(screen.getByText(ADVANCED_REGEX));
userEvent.click(screen.getByText(FILTER_SETTINGS_REGEX));
userEvent.click(getCheckbox(PRE_FILTER_REGEX));
await waitFor(() =>
expect(
Original file line number Diff line number Diff line change
@@ -175,13 +175,18 @@ export function FiltersConfigModal({
buildFilterGroup(filterHierarchy),
);

const getActiveFilterPanelKey = (filterId: string) => [
`${filterId}-${FilterPanels.configuration.key}`,
`${filterId}-${FilterPanels.settings.key}`,
];

const [activeFilterPanelKey, setActiveFilterPanelKey] = useState<
string | string[]
>(`${initialCurrentFilterId}-${FilterPanels.basic.key}`);
>(getActiveFilterPanelKey(initialCurrentFilterId));

const onTabChange = (filterId: string) => {
setCurrentFilterId(filterId);
setActiveFilterPanelKey(`${filterId}-${FilterPanels.basic.key}`);
setActiveFilterPanelKey(getActiveFilterPanelKey(filterId));
};

// generates a new filter id and appends it to the newFilterIds
@@ -196,7 +201,7 @@ export function FiltersConfigModal({
{ id: newFilterId, parentId: null },
]);
setOrderedFilters([...orderedFilters, [newFilterId]]);
setActiveFilterPanelKey(`${newFilterId}-${FilterPanels.basic.key}`);
setActiveFilterPanelKey(getActiveFilterPanelKey(newFilterId));
},
[
newFilterIds,
@@ -234,6 +239,9 @@ export function FiltersConfigModal({
setSaveAlertVisible(false);
setFormValues({ filters: {} });
setErroredFilters([]);
if (filterIds.length > 0) {
setActiveFilterPanelKey(getActiveFilterPanelKey(filterIds[0]));
}
if (!isSaving) {
const initialFilterHierarchy = getInitialFilterHierarchy();
setFilterHierarchy(initialFilterHierarchy);
@@ -464,7 +472,7 @@ export function FiltersConfigModal({
<StyledModalWrapper
visible={isOpen}
maskClosable={false}
title={t('Filters configuration and scoping')}
title={t('Add and edit filters')}
width="50%"
destroyOnClose
onCancel={handleCancel}
Original file line number Diff line number Diff line change
@@ -56,12 +56,11 @@ const config: ControlPanelConfig = {
name: 'multiSelect',
config: {
type: 'CheckboxControl',
label: t('Multiple select'),
label: t('Can select multiple values'),
default: multiSelect,
affectsDataMask: true,
resetConfig: true,
renderTrigger: true,
description: t('Allow selecting multiple values'),
},
},
],
@@ -70,10 +69,12 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t('User must select a value for this filter.'),
description: t(
'User must select a value before applying the filter',
),
},
},
],
Original file line number Diff line number Diff line change
@@ -52,10 +52,12 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t('User must select a value for this filter.'),
description: t(
'User must select a value before applying the filter',
),
},
},
{
11 changes: 5 additions & 6 deletions superset-frontend/src/filters/components/Select/controlPanel.ts
Original file line number Diff line number Diff line change
@@ -74,12 +74,11 @@ const config: ControlPanelConfig = {
name: 'multiSelect',
config: {
type: 'CheckboxControl',
label: t('Multiple select'),
label: t('Can select multiple values'),
default: multiSelect,
resetConfig: true,
affectsDataMask: true,
renderTrigger: true,
description: t('Allow selecting multiple values'),
},
},
],
@@ -88,7 +87,7 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: enableEmptyFilter,
renderTrigger: true,
description: t(
@@ -102,14 +101,14 @@ const config: ControlPanelConfig = {
name: 'defaultToFirstItem',
config: {
type: 'CheckboxControl',
label: t('Default to first item'),
label: t('Select first filter value by default'),
default: defaultToFirstItem,
resetConfig: true,
affectsDataMask: true,
renderTrigger: true,
requiredFirst: true,
description: t(
'Select first item by default (when using this option, default value can’t be set)',
'When using this option, default value can’t be set',
),
},
},
@@ -134,7 +133,7 @@ const config: ControlPanelConfig = {
type: 'CheckboxControl',
renderTrigger: true,
affectsDataMask: true,
label: t('Search all filter options'),
label: t('Dynamically search all filter values'),
default: searchAllOptions,
description: t(
'By default, each filter loads at most 1000 choices at the initial page load. ' +
2 changes: 1 addition & 1 deletion superset-frontend/src/filters/components/Select/types.ts
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ import { PluginFilterHooks, PluginFilterStylesProps } from '../types';

export type SelectValue = (number | string)[] | null | undefined;

interface PluginFilterSelectCustomizeProps {
export interface PluginFilterSelectCustomizeProps {
defaultValue?: SelectValue;
enableEmptyFilter: boolean;
inverseSelection: boolean;
6 changes: 4 additions & 2 deletions superset-frontend/src/filters/components/Time/controlPanel.ts
Original file line number Diff line number Diff line change
@@ -50,10 +50,12 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t('User must select a value for this filter.'),
description: t(
'User must select a value before applying the filter',
),
},
},
],
Original file line number Diff line number Diff line change
@@ -30,10 +30,12 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t('User must select a value for this filter.'),
description: t(
'User must select a value before applying the filter',
),
},
},
],
Original file line number Diff line number Diff line change
@@ -30,10 +30,12 @@ const config: ControlPanelConfig = {
name: 'enableEmptyFilter',
config: {
type: 'CheckboxControl',
label: t('Required'),
label: t('Filter value is required'),
default: false,
renderTrigger: true,
description: t('User must select a value for this filter.'),
description: t(
'User must select a value before applying the filter',
),
},
},
],

0 comments on commit fdf57cc

Please sign in to comment.