chore(FilterBar): move the "Add/edit filters" button in the FilterBar…

… to the settings menu (apache#31290) Co-authored-by: Geido <[email protected]> Co-authored-by: Diego Pucci <[email protected]>
preset-io · Dec 6, 2024 · 079e732 · 079e732
1 parent 48864ce
commit 079e732
Show file tree

Hide file tree

Showing 14 changed files with 68 additions and 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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/...dashboard/components/nativeFilters/FilterBar/FilterBarSettings/FilterBarSettings.test.tsx b/...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/...set-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx b/...set-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<RootState, boolean>(
     ({ dashboardInfo }) => dashboardInfo.dash_edit_perm,
   );
+  const filters = useFilters();
+  const filterValues = useMemo(() => Object.values(filters), [filters]);
+  const dashboardId = useSelector<RootState, number>(
+    ({ 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: (
+          <FilterConfigurationLink
+            dashboardId={dashboardId}
+            createNewOnOpen={filterValues.length === 0}
+          >
+            {t('Add or edit filters')}
+          </FilterConfigurationLink>
+        ),
+        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/...ontend/src/dashboard/components/nativeFilters/FilterBar/FilterConfigurationLink/index.tsx b/...ontend/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<FCBProps> = ({
   createNewOnOpen,
   dashboardId,
@@ -69,14 +63,9 @@ export const FilterConfigurationLink: FC<FCBProps> = ({
   return (
     <>
       {/* eslint-disable-next-line jsx-a11y/no-static-element-interactions */}
-      <HeaderButton
-        {...getFilterBarTestId('create-filter')}
-        buttonStyle="link"
-        buttonSize="xsmall"
-        onClick={handleClick}
-      >
+      <span {...getFilterBarTestId('create-filter')} onClick={handleClick}>
         {children}
-      </HeaderButton>
+      </span>
       <FiltersConfigModal
         isOpen={isOpen}
         onSave={submit}

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Header/index.tsx
@@ -18,13 +18,9 @@
  */
 /* eslint-disable no-param-reassign */
 import { css, styled, t, useTheme } from '@superset-ui/core';
-import { memo, FC, useMemo } from 'react';
+import { memo, 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 '../utils';
 import FilterBarSettings from '../FilterBarSettings';
 
@@ -62,7 +58,6 @@ const Wrapper = styled.div`
     padding: ${theme.gridUnit * 3}px ${theme.gridUnit * 2}px ${
       theme.gridUnit
     }px;
-
     .ant-dropdown-trigger span {
       padding-right: ${theme.gridUnit * 2}px;
     }
@@ -73,35 +68,8 @@ type HeaderProps = {
   toggleFiltersBar: (arg0: boolean) => 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<HeaderProps> = ({ toggleFiltersBar }) => {
   const theme = useTheme();
-  const filters = useFilters();
-  const filterValues = useMemo(() => Object.values(filters), [filters]);
-  const canEdit = useSelector<RootState, boolean>(
-    ({ dashboardInfo }) => dashboardInfo.dash_edit_perm,
-  );
-  const dashboardId = useSelector<RootState, number>(
-    ({ dashboardInfo }) => dashboardInfo.id,
-  );
 
   return (
     <Wrapper>
@@ -117,16 +85,6 @@ const Header: FC<HeaderProps> = ({ toggleFiltersBar }) => {
           <Icons.Expand iconColor={theme.colors.grayscale.base} />
         </HeaderButton>
       </TitleArea>
-      {canEdit && (
-        <AddFiltersButtonContainer>
-          <FilterConfigurationLink
-            dashboardId={dashboardId}
-            createNewOnOpen={filterValues.length === 0}
-          >
-            <Icons.PlusSmall /> {t('Add/Edit Filters')}
-          </FilterConfigurationLink>
-        </AddFiltersButtonContainer>
-      )}
     </Wrapper>
   );
 };