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 <diegopucci.me@gmail.com>
2024-12-06 20:52:55 +02:00 · 2024-12-06 20:52:55 +02:00 · 079e7327a2
parent 48864ce8c7
commit 079e7327a2
14 changed files with 68 additions and 140 deletions
--- 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);

--- 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',
      );
--- 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,
  });
--- 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"]',
--- 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'));
--- 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();
--- 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<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,
  ]);

--- 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<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}
--- 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>
  );
 };
--- 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<HorizontalBarProps> = ({
  actions,
-  canEdit,
-  dashboardId,
  dataMaskSelected,
  filterValues,
  isInitialized,
@ -141,16 +113,6 @@ const HorizontalFilterBar: FC<HorizontalBarProps> = ({
        ) : (
          <>
            <FilterBarSettings />
-            {canEdit && (
-              <FiltersLinkContainer hasFilters={hasFilters}>
-                <FilterConfigurationLink
-                  dashboardId={dashboardId}
-                  createNewOnOpen={filterValues.length === 0}
-                >
-                  <Icons.PlusSmall /> {t('Add/Edit Filters')}
-                </FilterConfigurationLink>
-              </FiltersLinkContainer>
-            )}
            {!hasFilters && (
              <FilterBarEmptyStateContainer data-test="horizontal-filterbar-empty">
                {t('No filters are currently added to this dashboard.')}
--- 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();
-});
--- 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<VerticalBarProps> = ({
            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',
              )
            }
          />
--- 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 }),
--- 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}
        >
-          <Icons.Edit iconSize="l" iconColor={theme.colors.grayscale.light1} />
+          <Icons.Edit
+            iconSize="l"
+            iconColor={theme.colors.grayscale.light1}
+            css={() => css`
+              cursor: pointer;
+            `}
+          />
        </FilterConfigurationLink>
      )}
    </Row>