From 079e7327a257a5cbfbd08616b64a3ca78d134051 Mon Sep 17 00:00:00 2001 From: alexandrusoare <37236580+alexandrusoare@users.noreply.github.com> Date: Fri, 6 Dec 2024 20:52:55 +0200 Subject: [PATCH] chore(FilterBar): move the "Add/edit filters" button in the FilterBar to the settings menu (#31290) Co-authored-by: Geido <60598000+geido@users.noreply.github.com> Co-authored-by: Diego Pucci --- .../e2e/dashboard/horizontalFilterBar.test.ts | 5 ++- .../e2e/dashboard/nativeFilters.test.ts | 3 ++ .../cypress/e2e/dashboard/utils.ts | 3 ++ .../cypress/support/directories.ts | 1 + .../FilterBar/FilterBar.test.tsx | 3 +- .../FilterBarSettings.test.tsx | 39 +++++++--------- .../FilterBar/FilterBarSettings/index.tsx | 25 ++++++++++- .../FilterConfigurationLink/index.tsx | 15 +------ .../nativeFilters/FilterBar/Header/index.tsx | 44 +------------------ .../nativeFilters/FilterBar/Horizontal.tsx | 40 +---------------- .../FilterBar/HorizontalFilterBar.test.tsx | 12 ----- .../nativeFilters/FilterBar/Vertical.tsx | 2 +- .../FilterCard/FilterCard.test.tsx | 8 ++-- .../nativeFilters/FilterCard/NameRow.tsx | 8 +++- 14 files changed, 68 insertions(+), 140 deletions(-) diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts index d9ed21258f20a..3cc1a2de6660e 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/horizontalFilterBar.test.ts @@ -88,6 +88,9 @@ describe('Horizontal FilterBar', () => { cy.getBySel('horizontal-filterbar-empty') .contains('No filters are currently added to this dashboard.') .should('exist'); + cy.get(nativeFilters.filtersPanel.filterGear).click({ + force: true, + }); cy.getBySel('filter-bar__create-filter').should('exist'); cy.getBySel('filterbar-action-buttons').should('exist'); }); @@ -120,7 +123,7 @@ describe('Horizontal FilterBar', () => { cy.getBySel('form-item-value').should('have.length', 3); cy.viewport(768, 1024); - cy.getBySel('form-item-value').should('have.length', 0); + cy.getBySel('form-item-value').should('have.length', 1); openMoreFilters(false); cy.getBySel('form-item-value').should('have.length', 3); diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts index 8f0987433e839..371de2da7c320 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts @@ -264,6 +264,9 @@ describe('Native filters', () => { it('User can expand / retract native filter sidebar on a dashboard', () => { expandFilterOnLeftPanel(); + cy.get(nativeFilters.filtersPanel.filterGear).click({ + force: true, + }); cy.get(nativeFilters.filterFromDashboardView.createFilterButton).should( 'be.visible', ); diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts index 5e7e8ba9af5ec..655eaecc8a76a 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts @@ -228,6 +228,9 @@ export function collapseFilterOnLeftPanel() { ************************************************************************* */ export function enterNativeFilterEditModal(waitForDataset = true) { interceptDataset(); + cy.get(nativeFilters.filtersPanel.filterGear).click({ + force: true, + }); cy.get(nativeFilters.filterFromDashboardView.createFilterButton).click({ force: true, }); diff --git a/superset-frontend/cypress-base/cypress/support/directories.ts b/superset-frontend/cypress-base/cypress/support/directories.ts index 618bfe2989033..77f455479c001 100644 --- a/superset-frontend/cypress-base/cypress/support/directories.ts +++ b/superset-frontend/cypress-base/cypress/support/directories.ts @@ -346,6 +346,7 @@ export const nativeFilters = { filterTypeInput: dataTestLocator('filters-config-modal__filter-type'), fieldInput: dataTestLocator('field-input'), filterTypeItem: '.ant-select-selection-item', + filterGear: dataTestLocator('filterbar-orientation-icon'), }, filterFromDashboardView: { filterValueInput: '[class="ant-select-selection-search-input"]', 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 1a6f92c0c2439..484b080a6b104 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx @@ -75,7 +75,8 @@ const FILTER_NAME = 'Time filter 1'; const addFilterFlow = async () => { // open filter config modal userEvent.click(screen.getByTestId(getTestId('collapsable'))); - userEvent.click(screen.getByTestId(getTestId('create-filter'))); + userEvent.click(screen.getByLabelText('gear')); + userEvent.click(screen.getByText('Add or edit filters')); // select filter userEvent.click(screen.getByText('Value')); userEvent.click(screen.getByText('Time range')); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx index f1b3282921441..e3a80d21e9f30 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx @@ -31,7 +31,7 @@ const initialState: { dashboardInfo: DashboardInfo } = { id: 1, userId: '1', metadata: { - native_filter_configuration: {}, + native_filter_configuration: [{}], chart_configuration: {}, global_chart_configuration: { scope: { rootPath: ['ROOT_ID'], excluded: [] }, @@ -81,15 +81,6 @@ test('Dropdown trigger renders with FF HORIZONTAL_FILTER_BAR on', async () => { expect(screen.getByLabelText('gear')).toBeVisible(); }); -test('Dropdown trigger does not render with FF HORIZONTAL_FILTER_BAR off', async () => { - // @ts-ignore - global.featureFlags = { - [FeatureFlag.HorizontalFilterBar]: false, - }; - await setup(); - expect(screen.queryByLabelText('gear')).not.toBeInTheDocument(); -}); - test('Dropdown trigger renders with dashboard edit permissions', async () => { // @ts-ignore global.featureFlags = { @@ -128,7 +119,9 @@ test('Dropdown trigger does not render with FF DASHBOARD_CROSS_FILTERS off', asy global.featureFlags = { [FeatureFlag.DashboardCrossFilters]: false, }; - await setup(); + await setup({ + dash_edit_perm: false, + }); expect(screen.queryByRole('img', { name: 'gear' })).not.toBeInTheDocument(); }); @@ -175,7 +168,7 @@ test('Popover opens with "Vertical" selected', async () => { expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[1]).getByLabelText('check'), + within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), ).toBeInTheDocument(); }); @@ -190,7 +183,7 @@ test('Popover opens with "Horizontal" selected', async () => { expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), + within(screen.getAllByRole('menuitem')[3]).getByLabelText('check'), ).toBeInTheDocument(); }); @@ -216,20 +209,20 @@ test('On selection change, send request and update checked value', async () => { expect(await screen.findByText('Vertical (Left)')).toBeInTheDocument(); expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[1]).getByLabelText('check'), + within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), ).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[2]).queryByLabelText('check'), + within(screen.getAllByRole('menuitem')[3]).queryByLabelText('check'), ).not.toBeInTheDocument(); userEvent.click(screen.getByText('Horizontal (Top)')); // 1st check - checkmark appears immediately after click expect( - await within(screen.getAllByRole('menuitem')[2]).findByLabelText('check'), + await within(screen.getAllByRole('menuitem')[3]).findByLabelText('check'), ).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[1]).queryByLabelText('check'), + within(screen.getAllByRole('menuitem')[2]).queryByLabelText('check'), ).not.toBeInTheDocument(); // successful query @@ -246,10 +239,10 @@ test('On selection change, send request and update checked value', async () => { // 2nd check - checkmark stays after successful query expect( - await within(screen.getAllByRole('menuitem')[2]).findByLabelText('check'), + await within(screen.getAllByRole('menuitem')[3]).findByLabelText('check'), ).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[1]).queryByLabelText('check'), + within(screen.getAllByRole('menuitem')[2]).queryByLabelText('check'), ).not.toBeInTheDocument(); fetchMock.reset(); @@ -273,10 +266,10 @@ test('On failed request, restore previous selection', async () => { expect(screen.getByText('Horizontal (Top)')).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[1]).getByLabelText('check'), + within(screen.getAllByRole('menuitem')[2]).getByLabelText('check'), ).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[2]).queryByLabelText('check'), + within(screen.getAllByRole('menuitem')[3]).queryByLabelText('check'), ).not.toBeInTheDocument(); userEvent.click(await screen.findByText('Horizontal (Top)')); @@ -294,10 +287,10 @@ test('On failed request, restore previous selection', async () => { // checkmark gets rolled back to the original selection after successful query expect( - await within(screen.getAllByRole('menuitem')[1]).findByLabelText('check'), + await within(screen.getAllByRole('menuitem')[2]).findByLabelText('check'), ).toBeInTheDocument(); expect( - within(screen.getAllByRole('menuitem')[2]).queryByLabelText('check'), + within(screen.getAllByRole('menuitem')[3]).queryByLabelText('check'), ).not.toBeInTheDocument(); fetchMock.reset(); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx index 9b12dbc164b36..9afc05b40c55f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx @@ -38,7 +38,9 @@ import DropdownSelectableIcon, { } from 'src/components/DropdownSelectableIcon'; import Checkbox from 'src/components/Checkbox'; import { clearDataMaskState } from 'src/dataMask/actions'; +import { useFilters } from 'src/dashboard/components/nativeFilters/FilterBar/state'; import { useCrossFiltersScopingModal } from '../CrossFilters/ScopingModal/useCrossFiltersScopingModal'; +import FilterConfigurationLink from '../FilterConfigurationLink'; type SelectedKey = FilterBarOrientation | string | number; @@ -65,6 +67,7 @@ const StyledCheckbox = styled(Checkbox)` const CROSS_FILTERS_MENU_KEY = 'cross-filters-menu-key'; const CROSS_FILTERS_SCOPING_MENU_KEY = 'cross-filters-scoping-menu-key'; +const ADD_EDIT_FILTERS_MENU_KEY = 'add-edit-filters-menu-key'; const isOrientation = (o: SelectedKey): o is FilterBarOrientation => o === FilterBarOrientation.Vertical || o === FilterBarOrientation.Horizontal; @@ -91,6 +94,11 @@ const FilterBarSettings = () => { const canEdit = useSelector( ({ dashboardInfo }) => dashboardInfo.dash_edit_perm, ); + const filters = useFilters(); + const filterValues = useMemo(() => Object.values(filters), [filters]); + const dashboardId = useSelector( + ({ dashboardInfo }) => dashboardInfo.id, + ); const canSetHorizontalFilterBar = canEdit && isFeatureEnabled(FeatureFlag.HorizontalFilterBar); @@ -166,6 +174,20 @@ const FilterBarSettings = () => { const menuItems = useMemo(() => { const items: DropDownSelectableProps['menuItems'] = []; + if (canEdit) { + items.push({ + key: ADD_EDIT_FILTERS_MENU_KEY, + label: ( + + {t('Add or edit filters')} + + ), + divider: canSetHorizontalFilterBar, + }); + } if (isCrossFiltersFeatureEnabled && canEdit) { items.push({ key: CROSS_FILTERS_MENU_KEY, @@ -177,7 +199,6 @@ const FilterBarSettings = () => { divider: canSetHorizontalFilterBar, }); } - if (canSetHorizontalFilterBar) { items.push({ key: 'placement', @@ -199,6 +220,8 @@ const FilterBarSettings = () => { canEdit, canSetHorizontalFilterBar, crossFiltersMenuItem, + dashboardId, + filterValues, isCrossFiltersFeatureEnabled, ]); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx index 7be1e7814c085..f8164d241a134 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx @@ -20,8 +20,6 @@ import { ReactNode, FC, useCallback, useState, memo } from 'react'; import { useDispatch } from 'react-redux'; import { setFilterConfiguration } from 'src/dashboard/actions/nativeFilters'; -import Button from 'src/components/Button'; -import { styled } from '@superset-ui/core'; import FiltersConfigModal from 'src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal'; import { getFilterBarTestId } from '../utils'; import { SaveFilterChangesType } from '../../FiltersConfigModal/types'; @@ -34,10 +32,6 @@ export interface FCBProps { children?: ReactNode; } -const HeaderButton = styled(Button)` - padding: 0; -`; - export const FilterConfigurationLink: FC = ({ createNewOnOpen, dashboardId, @@ -69,14 +63,9 @@ export const FilterConfigurationLink: FC = ({ return ( <> {/* eslint-disable-next-line jsx-a11y/no-static-element-interactions */} - + {children} - + void; }; -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 = useMemo(() => Object.values(filters), [filters]); - const canEdit = useSelector( - ({ dashboardInfo }) => dashboardInfo.dash_edit_perm, - ); - const dashboardId = useSelector( - ({ dashboardInfo }) => dashboardInfo.id, - ); return ( @@ -117,16 +85,6 @@ const Header: FC = ({ toggleFiltersBar }) => { - {canEdit && ( - - - {t('Add/Edit Filters')} - - - )} ); }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx index e97a8768e063b..0e5b82aed2651 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Horizontal.tsx @@ -25,7 +25,6 @@ import { styled, t, } from '@superset-ui/core'; -import Icons from 'src/components/Icons'; import Loading from 'src/components/Loading'; import { RootState } from 'src/dashboard/types'; import { useChartLayoutItems } from 'src/dashboard/util/useChartLayoutItems'; @@ -35,7 +34,6 @@ import FilterControls from './FilterControls/FilterControls'; import { useChartsVerboseMaps, getFilterBarTestId } from './utils'; import { HorizontalBarProps } from './types'; import FilterBarSettings from './FilterBarSettings'; -import FilterConfigurationLink from './FilterConfigurationLink'; import crossFiltersSelector from './CrossFilters/selectors'; import { CrossFilterIndicator } from '../selectors'; @@ -70,39 +68,13 @@ const FilterBarEmptyStateContainer = styled.div` font-weight: ${theme.typography.weights.bold}; color: ${theme.colors.grayscale.base}; font-size: ${theme.typography.sizes.s}px; - `} -`; - -const FiltersLinkContainer = styled.div<{ hasFilters: boolean }>` - ${({ theme, hasFilters }) => ` - height: 24px; - display: flex; - align-items: center; - padding: 0 ${theme.gridUnit * 4}px 0 ${theme.gridUnit * 4}px; - border-right: ${ - hasFilters ? `1px solid ${theme.colors.grayscale.light2}` : 0 - }; - - button { - display: flex; - align-items: center; - > .anticon { - height: 24px; - padding-right: ${theme.gridUnit}px; - } - > .anticon + span, > .anticon { - margin-right: 0; - margin-left: 0; - } - } + padding-left: ${theme.gridUnit * 2}px; `} `; const EMPTY_ARRAY: CrossFilterIndicator[] = []; const HorizontalFilterBar: FC = ({ actions, - canEdit, - dashboardId, dataMaskSelected, filterValues, isInitialized, @@ -141,16 +113,6 @@ const HorizontalFilterBar: FC = ({ ) : ( <> - {canEdit && ( - - - {t('Add/Edit Filters')} - - - )} {!hasFilters && ( {t('No filters are currently added to this dashboard.')} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx index 16c245e51c451..3201ffb1a547d 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/HorizontalFilterBar.test.tsx @@ -81,15 +81,3 @@ test('should render the loading icon', async () => { }); expect(screen.getByRole('status', { name: 'Loading' })).toBeInTheDocument(); }); - -test('should render Add/Edit Filters', async () => { - await renderWrapper(); - expect(screen.getByText('Add/Edit Filters')).toBeInTheDocument(); -}); - -test('should not render Add/Edit Filters', async () => { - await renderWrapper({ - canEdit: false, - }); - expect(screen.queryByText('Add/Edit Filters')).not.toBeInTheDocument(); -}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx index 9fd9a769b6d10..b76b65c13395c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx @@ -174,7 +174,7 @@ const VerticalFilterBar: FC = ({ description={ canEdit && t( - 'Click on "+Add/Edit Filters" button to create new dashboard filters', + 'Click on "Add or Edit Filters" option in Settings to create new dashboard filters', ) } /> diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx index 2f2f4e4525e42..e1ded0035e688 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/FilterCard.test.tsx @@ -306,9 +306,7 @@ test('focus filter on filter card dependency click', () => { test('edit filter button for dashboard viewer', () => { renderContent(); - expect( - screen.queryByRole('button', { name: /edit/i }), - ).not.toBeInTheDocument(); + expect(screen.queryByRole('img', { name: /edit/i })).not.toBeInTheDocument(); }); test('edit filter button for dashboard editor', () => { @@ -317,7 +315,7 @@ test('edit filter button for dashboard editor', () => { dashboardInfo: { dash_edit_perm: true }, }); - expect(screen.getByRole('button', { name: /edit/i })).toBeVisible(); + expect(screen.getByRole('img', { name: /edit/i })).toBeVisible(); }); test('open modal on edit filter button click', async () => { @@ -326,7 +324,7 @@ test('open modal on edit filter button click', async () => { dashboardInfo: { dash_edit_perm: true }, }); - const editButton = screen.getByRole('button', { name: /edit/i }); + const editButton = screen.getByRole('img', { name: /edit/i }); userEvent.click(editButton); expect( await screen.findByRole('dialog', { name: /add and edit filters/i }), diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx index 8022fa917c097..01679b19264a8 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterCard/NameRow.tsx @@ -62,7 +62,13 @@ export const NameRow = ({ onClick={hidePopover} initialFilterId={filter.id} > - + css` + cursor: pointer; + `} + /> )}