From 45815d8642fb9cd076d33682d6ba9df72cd81d6a Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 5 Dec 2024 09:13:24 -0700 Subject: [PATCH] fix(filters): improving the add filter/divider UI. (#31279) --- .../e2e/dashboard/nativeFilters.test.ts | 1 - .../cypress/e2e/dashboard/utils.ts | 6 +- .../cypress/support/directories.ts | 6 +- .../SqlLab/components/TableElement/index.tsx | 1 - .../src/components/ButtonGroup/index.tsx | 13 +++- .../src/components/Icons/AntdEnhanced.tsx | 2 + .../FilterConfigPane.test.tsx | 30 ++++---- .../FiltersConfigModal/FilterTitlePane.tsx | 73 +++++++++---------- .../NativeFiltersModal.test.tsx | 6 +- 9 files changed, 64 insertions(+), 74 deletions(-) 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 56a302acb..8f0987433 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/nativeFilters.test.ts @@ -263,7 +263,6 @@ describe('Native filters', () => { }); it('User can expand / retract native filter sidebar on a dashboard', () => { - cy.get(nativeFilters.addFilterButton.button).should('not.exist'); expandFilterOnLeftPanel(); 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 1fef88417..5e7e8ba9a 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts @@ -243,11 +243,7 @@ export function enterNativeFilterEditModal(waitForDataset = true) { * @summary helper for adding new filter ************************************************************************* */ export function clickOnAddFilterInModal() { - cy.get(nativeFilters.addFilterButton.button).first().click(); - return cy - .get(nativeFilters.addFilterButton.dropdownItem) - .contains('Filter') - .click({ force: true }); + return cy.get(nativeFilters.modal.addNewFilterButton).click({ force: true }); } /** ************************************************************************ diff --git a/superset-frontend/cypress-base/cypress/support/directories.ts b/superset-frontend/cypress-base/cypress/support/directories.ts index 52285b6dd..618bfe298 100644 --- a/superset-frontend/cypress-base/cypress/support/directories.ts +++ b/superset-frontend/cypress-base/cypress/support/directories.ts @@ -334,10 +334,8 @@ export const nativeFilters = { }, addFilter: dataTestLocator('add-filter-button'), defaultValueCheck: '.ant-checkbox-checked', - }, - addFilterButton: { - button: `.ant-modal-content [data-test="new-dropdown-icon"]`, - dropdownItem: '.ant-dropdown-menu-item', + addNewFilterButton: dataTestLocator('add-new-filter-button'), + addNewDividerButton: dataTestLocator('add-new-divider-button'), }, filtersPanel: { filterName: dataTestLocator('filters-config-modal__name-input'), diff --git a/superset-frontend/src/SqlLab/components/TableElement/index.tsx b/superset-frontend/src/SqlLab/components/TableElement/index.tsx index bb70587ca..5817e5662 100644 --- a/superset-frontend/src/SqlLab/components/TableElement/index.tsx +++ b/superset-frontend/src/SqlLab/components/TableElement/index.tsx @@ -267,7 +267,6 @@ const TableElement = ({ table, ...props }: TableElementProps) => { return ( :nth-of-type(1):not(:nth-last-of-type(1))': { borderTopRightRadius: 0, borderBottomRightRadius: 0, borderRight: 0, marginLeft: 0, }, - '& :not(:nth-of-type(1)):not(:nth-last-of-type(1))': { + '& > :not(:nth-of-type(1)):not(:nth-last-of-type(1))': { borderRadius: 0, borderRight: 0, marginLeft: 0, }, - '& :nth-last-of-type(1):not(:nth-of-type(1))': { + '& > :nth-last-of-type(1):not(:nth-of-type(1))': { borderTopLeftRadius: 0, borderBottomLeftRadius: 0, marginLeft: 0, }, + ...(props.expand && { + '& .superset-button': { + flexGrow: 1, + }, + }), }} > {children} diff --git a/superset-frontend/src/components/Icons/AntdEnhanced.tsx b/superset-frontend/src/components/Icons/AntdEnhanced.tsx index ce19dd862..4dcbc494a 100644 --- a/superset-frontend/src/components/Icons/AntdEnhanced.tsx +++ b/superset-frontend/src/components/Icons/AntdEnhanced.tsx @@ -54,6 +54,7 @@ import { LineChartOutlined, LoadingOutlined, MonitorOutlined, + PicCenterOutlined, PlusCircleOutlined, PlusOutlined, ReloadOutlined, @@ -106,6 +107,7 @@ const AntdIcons = { LineChartOutlined, LoadingOutlined, MonitorOutlined, + PicCenterOutlined, PlusCircleOutlined, PlusOutlined, ReloadOutlined, diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx index ec14f4a8a..b927a51b4 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx @@ -98,9 +98,7 @@ test('remove filter', async () => { test('add filter', async () => { defaultRender(); // First trash icon - const addButton = screen.getByText('Add filters and dividers')!; - fireEvent.mouseOver(addButton); - const addFilterButton = await screen.findByText('Filter'); + const addFilterButton = await screen.findByText('Add Filter'); await act(async () => { fireEvent( @@ -116,9 +114,7 @@ test('add filter', async () => { test('add divider', async () => { defaultRender(); - const addButton = screen.getByText('Add filters and dividers')!; - fireEvent.mouseOver(addButton); - const addFilterButton = await screen.findByText('Divider'); + const addFilterButton = await screen.findByText('Add Divider'); await act(async () => { fireEvent( addFilterButton, @@ -151,18 +147,18 @@ test('filter container should scroll to bottom when adding items', async () => { defaultRender(state, props); - const addButton = screen.getByText('Add filters and dividers')!; - fireEvent.mouseOver(addButton); - - const addFilterButton = await screen.findByText('Filter'); + const addFilterButton = await screen.findByText('Add Filter'); + // add enough filters to make it scroll in the next expectation. await act(async () => { - fireEvent( - addFilterButton, - new MouseEvent('click', { - bubbles: true, - cancelable: true, - }), - ); + for (let i = 0; i < 3; i += 1) { + fireEvent( + addFilterButton, + new MouseEvent('click', { + bubbles: true, + cancelable: true, + }), + ); + } }); const containerElement = screen.getByTestId('filter-title-container'); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx index 8014f3bbb..0a76a5264 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx @@ -19,8 +19,9 @@ import { useRef, FC } from 'react'; import { NativeFilterType, styled, t, useTheme } from '@superset-ui/core'; -import { AntdDropdown } from 'src/components'; -import { MainNav as Menu } from 'src/components/Menu'; +import { Button } from 'src/components'; +import Icons from 'src/components/Icons'; + import FilterTitleContainer from './FilterTitleContainer'; import { FilterRemoval } from './types'; @@ -37,27 +38,14 @@ interface Props { erroredFilters: string[]; } -const StyledAddBox = styled.div` - ${({ theme }) => ` - cursor: pointer; - margin: ${theme.gridUnit * 4}px; - color: ${theme.colors.primary.base}; - &:hover { - color: ${theme.colors.primary.dark1}; - } -`} -`; const TabsContainer = styled.div` height: 100%; display: flex; flex-direction: column; + padding: ${({ theme }) => theme.gridUnit * 3}px; + padding-top: 2px; `; -const options = [ - { label: t('Filter'), type: NativeFilterType.NativeFilter }, - { label: t('Divider'), type: NativeFilterType.Divider }, -]; - const FilterTitlePane: FC = ({ getFilterTitle, onChange, @@ -70,9 +58,10 @@ const FilterTitlePane: FC = ({ removedFilters, erroredFilters, }) => { - const filtersContainerRef = useRef(null); const theme = useTheme(); + const filtersContainerRef = useRef(null); + const handleOnAdd = (type: NativeFilterType) => { onAdd(type); setTimeout(() => { @@ -88,33 +77,12 @@ const FilterTitlePane: FC = ({ }); }, 0); }; - const menu = ( - - {options.map(item => ( - handleOnAdd(item.type)}> - {item.label} - - ))} - - ); return ( - - -
{' '} - {t('Add filters and dividers')} - -
= ({ restoreFilter={restoreFilter} />
+
+ + +
); }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx index 31f0545ee..dafb1988c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/NativeFiltersModal.test.tsx @@ -88,13 +88,11 @@ describe('createNewOnOpen', () => { test('shows correct alert message for unsaved filters', async () => { const onCancel = jest.fn(); - const { getByRole, getByTestId, findByRole } = setup({ + const { getByRole, getByTestId } = setup({ onCancel, createNewOnOpen: false, }); - fireEvent.mouseOver(getByTestId('new-dropdown-icon')); - const addFilterButton = await findByRole('menuitem', { name: 'Filter' }); - fireEvent.click(addFilterButton); + fireEvent.click(getByTestId('add-new-filter-button')); fireEvent.click(getByRole('button', { name: 'Cancel' })); expect(onCancel).toHaveBeenCalledTimes(0); expect(getByRole('alert')).toBeInTheDocument();