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

feat(dashboard): Add divider component in native filters #17410

Merged
merged 13 commits into from
Nov 24, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { Provider } from 'react-redux';
import { mockStore } from 'spec/fixtures/mockStore';
import { styledMount as mount } from 'spec/helpers/theming';
import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint';
import { Dropdown, Menu } from 'src/common/components';
import Alert from 'src/components/Alert';
import { FiltersConfigModal } from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal';

Expand Down Expand Up @@ -60,7 +61,7 @@ jest.mock('@superset-ui/core', () => ({
describe('FiltersConfigModal', () => {
const mockedProps = {
isOpen: true,
initialFilterId: 'DefaultsID',
initialFilterId: 'NATIVE_FILTER-1',
createNewOnOpen: true,
onCancel: jest.fn(),
onSave: jest.fn(),
Expand Down Expand Up @@ -112,9 +113,13 @@ describe('FiltersConfigModal', () => {
await waitForComponentToPaint(wrapper);
}

function addFilter() {
async function addFilter() {
act(() => {
wrapper.find('[aria-label="Add filter"]').at(0).simulate('click');
wrapper.find(Dropdown).at(0).simulate('mouseEnter');
});
await waitForComponentToPaint(wrapper, 300);
act(() => {
wrapper.find(Menu.Item).at(0).simulate('click');
});
}

Expand All @@ -124,7 +129,7 @@ describe('FiltersConfigModal', () => {
});

it('shows correct alert message for unsaved filters', async () => {
addFilter();
await addFilter();
await clickCancel();
expect(onCancel.mock.calls).toHaveLength(0);
expect(wrapper.find(Alert).text()).toContain(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { getChartIdsInFilterScope } from '../../util/activeDashboardFilters';
import findTabIndexByComponentId from '../../util/findTabIndexByComponentId';
import { findTabsWithChartsInScope } from '../nativeFilters/utils';
import { setInScopeStatusOfFilters } from '../../actions/nativeFilters';
import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils';

type DashboardContainerProps = {
topLevelTabs?: LayoutItem;
Expand Down Expand Up @@ -71,6 +72,7 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const filterScopes = Object.values(nativeFilters ?? {}).map(filter => ({
id: filter.id,
scope: filter.scope,
type: filter.type,
}));
useEffect(() => {
if (
Expand All @@ -80,6 +82,13 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
return;
}
const scopes = filterScopes.map(filterScope => {
if (filterScope.id.startsWith(NATIVE_FILTER_DIVIDER_PREFIX)) {
return {
filterId: filterScope.id,
tabsInScope: [],
chartsInScope: [],
};
}
const { scope } = filterScope;
const chartsInScope: number[] = getChartIdsInFilterScope({
filterScope: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import { DataMaskStateWithId, DataMaskType } from 'src/dataMask/types';
import { areObjectsEqual } from 'src/reduxUtils';
import { Layout } from '../../types';
import { getTreeCheckedItems } from '../nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils';
import { NativeFilterType } from '../nativeFilters/types';

export enum IndicatorStatus {
Unset = 'UNSET',
Expand Down Expand Up @@ -268,11 +269,13 @@ export const selectNativeIndicatorsForChart = (
nativeFilterIndicators =
nativeFilters &&
Object.values(nativeFilters)
.filter(nativeFilter =>
getTreeCheckedItems(nativeFilter.scope, dashboardLayout).some(
layoutItem =>
dashboardLayout[layoutItem]?.meta?.chartId === chartId,
),
.filter(
nativeFilter =>
nativeFilter.type === NativeFilterType.NATIVE_FILTER &&
getTreeCheckedItems(nativeFilter.scope, dashboardLayout).some(
layoutItem =>
dashboardLayout[layoutItem]?.meta?.chartId === chartId,
),
)
.map(nativeFilter => {
const column = nativeFilter.targets[0]?.column?.name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,12 @@ const addFilterFlow = async () => {
userEvent.click(screen.getByText('Time range'));
userEvent.type(screen.getByTestId(getModalTestId('name-input')), FILTER_NAME);
userEvent.click(screen.getByText('Save'));
await screen.findByText('All Filters (1)');
await screen.findByText('All filters (1)');
};

const addFilterSetFlow = async () => {
// add filter set
userEvent.click(screen.getByText('Filter Sets (0)'));
userEvent.click(screen.getByText('Filter sets (0)'));

// check description
expect(screen.getByText('Filters (1)')).toBeInTheDocument();
Expand Down Expand Up @@ -301,6 +301,40 @@ describe('FilterBar', () => {
expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
});

it('renders dividers', async () => {
const divider = {
id: 'NATIVE_FILTER_DIVIDER-1',
type: 'DIVIDER',
scope: {
rootPath: ['ROOT_ID'],
excluded: [],
},
title: 'Select time range',
description: 'Select year/month etc..',
chartsInScope: [],
tabsInScope: [],
};
const stateWithDivider = {
...stateWithoutNativeFilters,
nativeFilters: {
filters: {
'NATIVE_FILTER_DIVIDER-1': divider,
},
},
};

renderWrapper(openedBarProps, stateWithDivider);

const title = await screen.findByText('Select time range');
const description = await screen.findByText('Select year/month etc..');

expect(title.tagName).toBe('H3');
expect(description.tagName).toBe('P');
// Do not enable buttons if there are not filters
expect(screen.getByTestId(getTestId('clear-button'))).toBeDisabled();
expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
});

it('create filter and apply it flow', async () => {
// @ts-ignore
global.featureFlags = {
Expand Down Expand Up @@ -332,7 +366,7 @@ describe('FilterBar', () => {

// change filter
expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
userEvent.click(await screen.findByText('All Filters (1)'));
userEvent.click(await screen.findByText('All filters (1)'));
await changeFilterValue();
await waitFor(() => expect(screen.getAllByText('Last day').length).toBe(2));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ import {
useDashboardHasTabs,
useSelectFiltersInScope,
} from 'src/dashboard/components/nativeFilters/state';
import { Filter } from 'src/dashboard/components/nativeFilters/types';
import {
Filter,
NativeFilterType,
} from 'src/dashboard/components/nativeFilters/types';
import CascadePopover from '../CascadeFilters/CascadePopover';
import { useFilters } from '../state';
import { buildCascadeFiltersTree } from './utils';
Expand Down Expand Up @@ -79,21 +82,32 @@ const FilterControls: FC<FilterControlsProps> = ({
const showCollapsePanel = dashboardHasTabs && cascadeFilters.length > 0;

const cascadePopoverFactory = useCallback(
index => (
<CascadePopover
data-test="cascade-filters-control"
key={cascadeFilters[index].id}
dataMaskSelected={dataMaskSelected}
visible={visiblePopoverId === cascadeFilters[index].id}
onVisibleChange={visible =>
setVisiblePopoverId(visible ? cascadeFilters[index].id : null)
}
filter={cascadeFilters[index]}
onFilterSelectionChange={onFilterSelectionChange}
directPathToChild={directPathToChild}
inView={false}
/>
),
index => {
const filter = cascadeFilters[index];
if (filter.type === NativeFilterType.DIVIDER) {
return (
<div>
<h3>{filter.title}</h3>
<p>{filter.description}</p>
</div>
);
}
return (
<CascadePopover
data-test="cascade-filters-control"
key={filter.id}
dataMaskSelected={dataMaskSelected}
visible={visiblePopoverId === filter.id}
onVisibleChange={visible =>
setVisiblePopoverId(visible ? filter.id : null)
}
filter={filter}
onFilterSelectionChange={onFilterSelectionChange}
directPathToChild={directPathToChild}
inView={false}
/>
);
},
[
cascadeFilters,
JSON.stringify(dataMaskSelected),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ import {
setFocusedNativeFilter,
unsetFocusedNativeFilter,
} from 'src/dashboard/actions/nativeFilters';
import { Filter } from '../../types';
import { Filter, NativeFilterType, Divider } from '../../types';
import { CascadeFilter } from '../CascadeFilters/types';
import { mapParentFiltersToChildren } from '../utils';

// eslint-disable-next-line import/prefer-default-export
export function buildCascadeFiltersTree(filters: Filter[]): CascadeFilter[] {
export function buildCascadeFiltersTree(
filters: Array<Divider | Filter>,
): Array<CascadeFilter | Divider> {
const cascadeChildren = mapParentFiltersToChildren(filters);

const getCascadeFilter = (filter: Filter): CascadeFilter => {
Expand All @@ -39,7 +41,11 @@ export function buildCascadeFiltersTree(filters: Filter[]): CascadeFilter[] {
};

return filters
.filter(filter => !filter.cascadeParentIds?.length)
.filter(
filter =>
filter.type === NativeFilterType.DIVIDER ||
!(filter as Filter).cascadeParentIds?.length,
)
.map(getCascadeFilter);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { FilterSet } from 'src/dashboard/reducers/types';
import { getFilterValueForDisplay } from './utils';
import { useFilters } from '../state';
import { getFilterBarTestId } from '../index';
import { NativeFilterType } from '../../types';

const FilterHeader = styled.div`
display: flex;
Expand Down Expand Up @@ -68,7 +69,9 @@ export type FiltersHeaderProps = {
const FiltersHeader: FC<FiltersHeaderProps> = ({ dataMask, filterSet }) => {
const theme = useTheme();
const filters = useFilters();
const filterValues = Object.values(filters);
const filterValues = Object.values(filters).filter(
nativeFilter => nativeFilter.type === NativeFilterType.NATIVE_FILTER,
);

let resultFilters = filterValues ?? [];
if (filterSet?.nativeFilters) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ import { DataMaskStateWithId, DataMaskWithId } from 'src/dataMask/types';
import { useImmer } from 'use-immer';
import { isEmpty, isEqual } from 'lodash';
import { testWithId } from 'src/utils/testUtils';
import { Filter } from 'src/dashboard/components/nativeFilters/types';
import {
Filter,
NativeFilterType,
} from 'src/dashboard/components/nativeFilters/types';
import Loading from 'src/components/Loading';
import { getInitialDataMask } from 'src/dataMask/reducer';
import { URL_PARAMS } from 'src/constants';
Expand Down Expand Up @@ -82,7 +85,6 @@ const Bar = styled.div<{ width: number }>`
border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
min-height: 100%;
display: none;

&.open {
display: flex;
}
Expand All @@ -97,14 +99,12 @@ const CollapsedBar = styled.div<{ offset: number }>`
padding-top: ${({ theme }) => theme.gridUnit * 2}px;
display: none;
text-align: center;

&.open {
display: flex;
flex-direction: column;
align-items: center;
padding: ${({ theme }) => theme.gridUnit * 2}px;
}

svg {
cursor: pointer;
}
Expand Down Expand Up @@ -278,9 +278,12 @@ const FilterBar: React.FC<FiltersBarProps> = ({
filterValues,
);
const isInitialized = useInitialization();

const tabPaneStyle = useMemo(() => ({ overflow: 'auto', height }), [height]);

const numberOfFilters = filterValues.filter(
filterValue => filterValue.type === NativeFilterType.NATIVE_FILTER,
).length;

return (
<BarWrapper
{...getFilterBarTestId()}
Expand Down Expand Up @@ -320,7 +323,7 @@ const FilterBar: React.FC<FiltersBarProps> = ({
activeKey={editFilterSetId ? TabIds.AllFilters : undefined}
>
<Tabs.TabPane
tab={t(`All Filters (${filterValues.length})`)}
tab={`${t('All filters')} (${numberOfFilters})`}
Copy link
Member

Choose a reason for hiding this comment

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

i know you didn't add this (and instead are just touching it to update the casing of the string), but this translation string isn't correct. you need something like:

t('All filters (%(numberOfFilters))', numberOfFilters)

see here for more details: https://github.com/apache-superset/superset-ui/tree/master/packages/superset-ui-core/src/translation#api

you'll also want to change the one on line 346

key={TabIds.AllFilters}
css={tabPaneStyle}
>
Expand All @@ -340,7 +343,7 @@ const FilterBar: React.FC<FiltersBarProps> = ({
</Tabs.TabPane>
<Tabs.TabPane
disabled={!!editFilterSetId}
tab={t(`Filter Sets (${filterSetFilterValues.length})`)}
tab={`${t('Filter sets')} (${filterSetFilterValues.length})`}
key={TabIds.FilterSets}
css={tabPaneStyle}
>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,20 @@
import { DataMaskStateWithId } from 'src/dataMask/types';
import { areObjectsEqual } from 'src/reduxUtils';
import { FilterState } from '@superset-ui/core';
import { Filter } from '../types';
import { Filter, Divider } from '../types';

export enum TabIds {
AllFilters = 'allFilters',
FilterSets = 'filterSets',
}

export function mapParentFiltersToChildren(filters: Filter[]): {
export function mapParentFiltersToChildren(filters: Array<Filter | Divider>): {
[id: string]: Filter[];
} {
const cascadeChildren = {};
filters.forEach(filter => {
const [parentId] = filter.cascadeParentIds || [];
const [parentId] =
('cascadeParentIds' in filter && filter.cascadeParentIds) || [];
if (parentId) {
if (!cascadeChildren[parentId]) {
cascadeChildren[parentId] = [];
Expand Down
Loading