From 9d5c0505cf9bf67be499abd4829195adf6ad17d5 Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Tue, 22 Feb 2022 17:45:53 +0100 Subject: [PATCH] feat(native-filters): Re-arrange controls in FilterBar (#18784) * feat(native-filters): Re-arrange controls in FilterBar * Typo fix * Add tests * Fix tests * Address PR reviews * Add 1px bottom padding to icon * Fix test * Tiny refactor * Change buttons background * Add hover state to Clear all button * Fix Clear All button logic so it clears all selections * Remove redundant useEffect * Set zindex higher than loading icon * Fix scrolling issue --- .../cypress/support/directories.ts | 2 +- .../src/components/Button/index.tsx | 8 +- .../DashboardBuilder/DashboardBuilder.tsx | 19 +-- .../ActionButtons/ActionButtons.test.tsx | 79 +++++++++++ .../FilterBar/ActionButtons/index.tsx | 125 ++++++++++++++++++ .../FilterBar/FilterBar.test.tsx | 4 +- .../FilterControls/FilterControls.tsx | 3 + .../FilterBar/FilterSets/index.tsx | 2 + .../FilterBar/Header/Header.test.tsx | 60 --------- .../nativeFilters/FilterBar/Header/index.tsx | 108 +++++---------- .../nativeFilters/FilterBar/index.tsx | 61 +++++---- superset-frontend/src/dashboard/constants.ts | 9 ++ superset-frontend/src/utils/common.js | 2 + 13 files changed, 308 insertions(+), 174 deletions(-) create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/ActionButtons.test.tsx create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx diff --git a/superset-frontend/cypress-base/cypress/support/directories.ts b/superset-frontend/cypress-base/cypress/support/directories.ts index 6d5838f1ae6f..b68ec36c74bf 100644 --- a/superset-frontend/cypress-base/cypress/support/directories.ts +++ b/superset-frontend/cypress-base/cypress/support/directories.ts @@ -343,7 +343,7 @@ export const nativeFilters = { collapse: dataTestLocator('filter-bar__collapse-button'), filterName: dataTestLocator('filter-control-name'), filterContent: '.ant-select-selection-item', - createFilterButton: dataTestLocator('create-filter'), + createFilterButton: dataTestLocator('filter-bar__create-filter'), timeRangeFilterContent: dataTestLocator('time-range-trigger'), }, createFilterButton: dataTestLocator('filter-bar__create-filter'), diff --git a/superset-frontend/src/components/Button/index.tsx b/superset-frontend/src/components/Button/index.tsx index 725ac6dd5dfa..8851a6f9cd68 100644 --- a/superset-frontend/src/components/Button/index.tsx +++ b/superset-frontend/src/components/Button/index.tsx @@ -202,14 +202,16 @@ export default function Button(props: ButtonProps) { }, '&[disabled], &[disabled]:hover': { color: grayscale.base, - backgroundColor: backgroundColorDisabled, - borderColor: borderColorDisabled, + backgroundColor: + buttonStyle === 'link' ? 'transparent' : backgroundColorDisabled, + borderColor: + buttonStyle === 'link' ? 'transparent' : borderColorDisabled, }, marginLeft: 0, '& + .superset-button': { marginLeft: theme.gridUnit * 2, }, - '& :first-of-type': { + '& > :first-of-type': { marginRight: firstChildMargin, }, }} diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx index 577351a389db..e354662c55c0 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx @@ -51,19 +51,20 @@ import FilterBar from 'src/dashboard/components/nativeFilters/FilterBar'; import Loading from 'src/components/Loading'; import { EmptyStateBig } from 'src/components/EmptyState'; import { useUiConfig } from 'src/components/UiConfigContext'; +import { + BUILDER_SIDEPANEL_WIDTH, + CLOSED_FILTER_BAR_WIDTH, + FILTER_BAR_HEADER_HEIGHT, + FILTER_BAR_TABS_HEIGHT, + HEADER_HEIGHT, + MAIN_HEADER_HEIGHT, + OPEN_FILTER_BAR_WIDTH, + TABS_HEIGHT, +} from 'src/dashboard/constants'; import { shouldFocusTabs, getRootLevelTabsComponent } from './utils'; import DashboardContainer from './DashboardContainer'; import { useNativeFilters } from './state'; -const MAIN_HEADER_HEIGHT = 53; -const TABS_HEIGHT = 50; -const HEADER_HEIGHT = 72; -const CLOSED_FILTER_BAR_WIDTH = 32; -const OPEN_FILTER_BAR_WIDTH = 260; -const FILTER_BAR_HEADER_HEIGHT = 80; -const FILTER_BAR_TABS_HEIGHT = 46; -const BUILDER_SIDEPANEL_WIDTH = 374; - type DashboardBuilderProps = {}; const StyledDiv = styled.div` diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/ActionButtons.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/ActionButtons.test.tsx new file mode 100644 index 000000000000..77aede5be353 --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/ActionButtons.test.tsx @@ -0,0 +1,79 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React from 'react'; +import userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; +import { ActionButtons } from './index'; + +const createProps = () => ({ + onApply: jest.fn(), + onClearAll: jest.fn(), + dataMaskSelected: { + DefaultsID: { + filterState: { + value: null, + }, + }, + }, + dataMaskApplied: { + DefaultsID: { + id: 'DefaultsID', + filterState: { + value: null, + }, + }, + }, + isApplyDisabled: false, +}); + +test('should render the "Apply" button', () => { + const mockedProps = createProps(); + render(, { useRedux: true }); + expect(screen.getByText('Apply filters')).toBeInTheDocument(); + expect(screen.getByText('Apply filters').parentElement).toBeEnabled(); +}); + +test('should render the "Clear all" button as disabled', () => { + const mockedProps = createProps(); + render(, { useRedux: true }); + const clearBtn = screen.getByText('Clear all'); + expect(clearBtn.parentElement).toBeDisabled(); +}); + +test('should render the "Apply" button as disabled', () => { + const mockedProps = createProps(); + const applyDisabledProps = { + ...mockedProps, + isApplyDisabled: true, + }; + render(, { useRedux: true }); + const applyBtn = screen.getByText('Apply filters'); + expect(applyBtn.parentElement).toBeDisabled(); + userEvent.click(applyBtn); + expect(mockedProps.onApply).not.toHaveBeenCalled(); +}); + +test('should apply', () => { + const mockedProps = createProps(); + render(, { useRedux: true }); + const applyBtn = screen.getByText('Apply filters'); + expect(mockedProps.onApply).not.toHaveBeenCalled(); + userEvent.click(applyBtn); + expect(mockedProps.onApply).toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx new file mode 100644 index 000000000000..55d4783a6e89 --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/ActionButtons/index.tsx @@ -0,0 +1,125 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import React, { useMemo } from 'react'; +import { + css, + DataMaskState, + DataMaskStateWithId, + styled, + t, +} from '@superset-ui/core'; +import Button from 'src/components/Button'; +import { isNullish } from 'src/utils/common'; +import { OPEN_FILTER_BAR_WIDTH } from 'src/dashboard/constants'; +import { getFilterBarTestId } from '../index'; + +interface ActionButtonsProps { + onApply: () => void; + onClearAll: () => void; + dataMaskSelected: DataMaskState; + dataMaskApplied: DataMaskStateWithId; + isApplyDisabled: boolean; +} + +const ActionButtonsContainer = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + align-items: center; + + position: fixed; + z-index: 100; + + // filter bar width minus 1px for border + width: ${OPEN_FILTER_BAR_WIDTH - 1}px; + bottom: 0; + + padding: ${theme.gridUnit * 4}px; + padding-top: ${theme.gridUnit * 6}px; + + background: linear-gradient(transparent, white 25%); + + pointer-events: none; + + & > button { + pointer-events: auto; + } + + & > .filter-apply-button { + margin-bottom: ${theme.gridUnit * 3}px; + } + + && > .filter-clear-all-button { + color: ${theme.colors.grayscale.base}; + margin-left: 0; + &:hover { + color: ${theme.colors.primary.dark1}; + } + + &[disabled], + &[disabled]:hover { + color: ${theme.colors.grayscale.light1}; + } + } + `}; +`; + +export const ActionButtons = ({ + onApply, + onClearAll, + dataMaskApplied, + dataMaskSelected, + isApplyDisabled, +}: ActionButtonsProps) => { + const isClearAllEnabled = useMemo( + () => + Object.values(dataMaskApplied).some( + filter => + !isNullish(dataMaskSelected[filter.id]?.filterState?.value) || + (!dataMaskSelected[filter.id] && + !isNullish(filter.filterState?.value)), + ), + [dataMaskApplied, dataMaskSelected], + ); + + return ( + + + + + ); +}; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx index 9be4a35cba09..632f8978efa2 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx @@ -241,9 +241,9 @@ describe('FilterBar', () => { expect(screen.getByText('Clear all')).toBeInTheDocument(); }); - it('should render the "Apply" option', () => { + it('should render the "Apply filters" option', () => { renderWrapper(); - expect(screen.getByText('Apply')).toBeInTheDocument(); + expect(screen.getByText('Apply filters')).toBeInTheDocument(); }); it('should render the collapse icon', () => { diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx index fb83fc96ea3d..dd1bfd971103 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx @@ -42,6 +42,9 @@ import { buildCascadeFiltersTree } from './utils'; const Wrapper = styled.div` padding: ${({ theme }) => theme.gridUnit * 4}px; + // 108px padding to make room for buttons with position: absolute + padding-bottom: ${({ theme }) => theme.gridUnit * 27}px; + &:hover { cursor: pointer; } diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/index.tsx index 18f5a5688be3..5fdfd481719e 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterSets/index.tsx @@ -48,6 +48,8 @@ const FilterSetsWrapper = styled.div` align-items: center; justify-content: center; grid-template-columns: 1fr; + // 108px padding to make room for buttons with position: absolute + padding-bottom: ${({ theme }) => theme.gridUnit * 27}px; & button.superset-button { margin-left: 0; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/Header.test.tsx index 64a2dd1d43a9..caa8efa0f7f1 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/Header.test.tsx @@ -23,24 +23,6 @@ import Header from './index'; const createProps = () => ({ toggleFiltersBar: jest.fn(), - onApply: jest.fn(), - onClearAll: jest.fn(), - dataMaskSelected: { - DefaultsID: { - filterState: { - value: null, - }, - }, - }, - dataMaskApplied: { - DefaultsID: { - id: 'DefaultsID', - filterState: { - value: null, - }, - }, - }, - isApplyDisabled: false, }); test('should render', () => { @@ -55,48 +37,6 @@ test('should render the "Filters" heading', () => { expect(screen.getByText('Filters')).toBeInTheDocument(); }); -test('should render the "Clear all" option', () => { - const mockedProps = createProps(); - render(
, { useRedux: true }); - expect(screen.getByText('Clear all')).toBeInTheDocument(); -}); - -test('should render the "Apply" button', () => { - const mockedProps = createProps(); - render(
, { useRedux: true }); - expect(screen.getByText('Apply')).toBeInTheDocument(); - expect(screen.getByText('Apply').parentElement).toBeEnabled(); -}); - -test('should render the "Clear all" button as disabled', () => { - const mockedProps = createProps(); - render(
, { useRedux: true }); - const clearBtn = screen.getByText('Clear all'); - expect(clearBtn.parentElement).toBeDisabled(); -}); - -test('should render the "Apply" button as disabled', () => { - const mockedProps = createProps(); - const applyDisabledProps = { - ...mockedProps, - isApplyDisabled: true, - }; - render(
, { useRedux: true }); - const applyBtn = screen.getByText('Apply'); - expect(applyBtn.parentElement).toBeDisabled(); - userEvent.click(applyBtn); - expect(mockedProps.onApply).not.toHaveBeenCalled(); -}); - -test('should apply', () => { - const mockedProps = createProps(); - render(
, { useRedux: true }); - const applyBtn = screen.getByText('Apply'); - expect(mockedProps.onApply).not.toHaveBeenCalled(); - userEvent.click(applyBtn); - expect(mockedProps.onApply).toHaveBeenCalled(); -}); - test('should render the expand button', () => { const mockedProps = createProps(); render(
, { useRedux: true }); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx index 2aac16e5e86b..86895b6a4f8f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx @@ -17,22 +17,15 @@ * under the License. */ /* eslint-disable no-param-reassign */ -import { - DataMaskState, - DataMaskStateWithId, - Filter, - styled, - t, - useTheme, -} from '@superset-ui/core'; +import { css, Filter, styled, t, useTheme } from '@superset-ui/core'; import React, { FC } from 'react'; import Icons from 'src/components/Icons'; import Button from 'src/components/Button'; import { useSelector } from 'react-redux'; import FilterConfigurationLink from 'src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink'; import { useFilters } from 'src/dashboard/components/nativeFilters/FilterBar/state'; +import { RootState } from 'src/dashboard/types'; import { getFilterBarTestId } from '..'; -import { RootState } from '../../../../types'; const TitleArea = styled.h4` display: flex; @@ -46,20 +39,6 @@ const TitleArea = styled.h4` } `; -const ActionButtons = styled.div` - display: grid; - flex-direction: row; - justify-content: center; - align-items: center; - grid-gap: 10px; - grid-template-columns: 1fr 1fr; - ${({ theme }) => `padding: 0 ${theme.gridUnit * 2}px`}; - - .btn { - flex: 1; - } -`; - const HeaderButton = styled(Button)` padding: 0; `; @@ -71,21 +50,28 @@ const Wrapper = styled.div` type HeaderProps = { toggleFiltersBar: (arg0: boolean) => void; - onApply: () => void; - onClearAll: () => void; - dataMaskSelected: DataMaskState; - dataMaskApplied: DataMaskStateWithId; - isApplyDisabled: boolean; }; -const Header: FC = ({ - onApply, - onClearAll, - isApplyDisabled, - dataMaskSelected, - dataMaskApplied, - toggleFiltersBar, -}) => { +const AddFiltersButtonContainer = styled.div` + ${({ theme }) => css` + margin-top: ${theme.gridUnit * 2}px; + + & button > [role='img']:first-of-type { + margin-right: ${theme.gridUnit}px; + line-height: 0; + } + + span[role='img'] { + padding-bottom: 1px; + } + + .ant-btn > .anticon + span { + margin-left: 0; + } + `} +`; + +const Header: FC = ({ toggleFiltersBar }) => { const theme = useTheme(); const filters = useFilters(); const filterValues = Object.values(filters); @@ -96,27 +82,10 @@ const Header: FC = ({ ({ dashboardInfo }) => dashboardInfo.id, ); - const isClearAllDisabled = Object.values(dataMaskApplied).every( - filter => - dataMaskSelected[filter.id]?.filterState?.value === null || - (!dataMaskSelected[filter.id] && filter.filterState?.value === null), - ); - return ( {t('Filters')} - {canEdit && ( - - - - )} = ({ - - - - + {canEdit && ( + + + {t('Add/Edit Filters')} + + + )} ); }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx index d324984d7cfc..e035ee02dedf 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx @@ -61,6 +61,7 @@ import EditSection from './FilterSets/EditSection'; import Header from './Header'; import FilterControls from './FilterControls/FilterControls'; import { RootState } from '../../../types'; +import { ActionButtons } from './ActionButtons'; export const FILTER_BAR_TEST_ID = 'filter-bar'; export const getFilterBarTestId = testWithId(FILTER_BAR_TEST_ID); @@ -168,7 +169,7 @@ const publishDataMask = debounce( const { search } = location; const previousParams = new URLSearchParams(search); const newParams = new URLSearchParams(); - let dataMaskKey = ''; + let dataMaskKey: string; previousParams.forEach((value, key) => { if (key !== URL_PARAMS.nativeFilters.name) { newParams.append(key, value); @@ -254,7 +255,7 @@ const FilterBar: React.FC = ({ }; }); }, - [dataMaskSelected, dispatch, setDataMaskSelected, tab], + [dataMaskSelected, dispatch, setDataMaskSelected], ); useEffect(() => { @@ -284,10 +285,6 @@ const FilterBar: React.FC = ({ } }, [JSON.stringify(filters), JSON.stringify(previousFilters)]); - useEffect(() => { - setDataMaskSelected(() => dataMaskApplied); - }, [JSON.stringify(dataMaskApplied), setDataMaskSelected]); - const dataMaskAppliedText = JSON.stringify(dataMaskApplied); useEffect(() => { publishDataMask(history, dashboardId, updateKey, dataMaskApplied, tabId); @@ -309,9 +306,14 @@ const FilterBar: React.FC = ({ filterIds.forEach(filterId => { if (dataMaskSelected[filterId]) { dispatch(clearDataMask(filterId)); + setDataMaskSelected(draft => { + if (draft[filterId].filterState?.value !== undefined) { + draft[filterId].filterState!.value = undefined; + } + }); } }); - }, [dataMaskSelected, dispatch]); + }, [dataMaskSelected, dispatch, setDataMaskSelected]); const openFiltersBar = useCallback( () => toggleFiltersBar(true), @@ -350,14 +352,7 @@ const FilterBar: React.FC = ({ -
+
{!isInitialized ? (
@@ -384,11 +379,26 @@ const FilterBar: React.FC = ({ filterSetId={editFilterSetId} /> )} - + {filterValues.length === 0 ? ( + + + + ) : ( + + )} = ({ image="filter.svg" description={ canEdit && - t( - 'Click the pencil icon above to add a filter to the dashboard', - ) + t('Click the button above to add a filter to the dashboard') } /> @@ -431,6 +439,13 @@ const FilterBar: React.FC = ({ )}
)} + ); diff --git a/superset-frontend/src/dashboard/constants.ts b/superset-frontend/src/dashboard/constants.ts index ab3fa29a0b77..169a2d09de19 100644 --- a/superset-frontend/src/dashboard/constants.ts +++ b/superset-frontend/src/dashboard/constants.ts @@ -33,3 +33,12 @@ export const PLACEHOLDER_DATASOURCE: Datasource = { main_dttm_col: '', description: '', }; + +export const MAIN_HEADER_HEIGHT = 53; +export const TABS_HEIGHT = 50; +export const HEADER_HEIGHT = 72; +export const CLOSED_FILTER_BAR_WIDTH = 32; +export const OPEN_FILTER_BAR_WIDTH = 260; +export const FILTER_BAR_HEADER_HEIGHT = 80; +export const FILTER_BAR_TABS_HEIGHT = 46; +export const BUILDER_SIDEPANEL_WIDTH = 374; diff --git a/superset-frontend/src/utils/common.js b/superset-frontend/src/utils/common.js index 26fdfb4bd6e4..4efdb205e5a9 100644 --- a/superset-frontend/src/utils/common.js +++ b/superset-frontend/src/utils/common.js @@ -144,3 +144,5 @@ export const detectOS = () => { return 'Unknown OS'; }; + +export const isNullish = value => value === null || value === undefined;