Skip to content

Commit

Permalink
fix(filters): improving the add filter/divider UI. (apache#31279)
Browse files Browse the repository at this point in the history
  • Loading branch information
rusackas authored Dec 5, 2024
1 parent cf5c770 commit 45815d8
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 74 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}

/** ************************************************************************
Expand Down
6 changes: 2 additions & 4 deletions superset-frontend/cypress-base/cypress/support/directories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,6 @@ const TableElement = ({ table, ...props }: TableElementProps) => {
return (
<ButtonGroup
css={css`
display: flex;
column-gap: ${theme.gridUnit * 1.5}px;
margin-right: ${theme.gridUnit}px;
& span {
Expand Down
13 changes: 10 additions & 3 deletions superset-frontend/src/components/ButtonGroup/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { ReactNode } from 'react';
export interface ButtonGroupProps {
className?: string;
children: ReactNode;
expand?: boolean;
}

export default function ButtonGroup(props: ButtonGroupProps) {
Expand All @@ -30,22 +31,28 @@ export default function ButtonGroup(props: ButtonGroupProps) {
role="group"
className={className}
css={{
'& :nth-of-type(1):not(:nth-last-of-type(1))': {
display: 'flex',
'& > :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}
Expand Down
2 changes: 2 additions & 0 deletions superset-frontend/src/components/Icons/AntdEnhanced.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
LineChartOutlined,
LoadingOutlined,
MonitorOutlined,
PicCenterOutlined,
PlusCircleOutlined,
PlusOutlined,
ReloadOutlined,
Expand Down Expand Up @@ -106,6 +107,7 @@ const AntdIcons = {
LineChartOutlined,
LoadingOutlined,
MonitorOutlined,
PicCenterOutlined,
PlusCircleOutlined,
PlusOutlined,
ReloadOutlined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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,
Expand Down Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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<Props> = ({
getFilterTitle,
onChange,
Expand All @@ -70,9 +58,10 @@ const FilterTitlePane: FC<Props> = ({
removedFilters,
erroredFilters,
}) => {
const filtersContainerRef = useRef<HTMLDivElement>(null);
const theme = useTheme();

const filtersContainerRef = useRef<HTMLDivElement>(null);

const handleOnAdd = (type: NativeFilterType) => {
onAdd(type);
setTimeout(() => {
Expand All @@ -88,33 +77,12 @@ const FilterTitlePane: FC<Props> = ({
});
}, 0);
};
const menu = (
<Menu mode="horizontal">
{options.map(item => (
<Menu.Item onClick={() => handleOnAdd(item.type)}>
{item.label}
</Menu.Item>
))}
</Menu>
);
return (
<TabsContainer>
<AntdDropdown
overlay={menu}
arrow
placement="topLeft"
trigger={['hover']}
>
<StyledAddBox>
<div data-test="new-dropdown-icon" className="fa fa-plus" />{' '}
<span>{t('Add filters and dividers')}</span>
</StyledAddBox>
</AntdDropdown>
<div
css={{
height: '100%',
overflowY: 'auto',
marginLeft: theme.gridUnit * 3,
}}
>
<FilterTitleContainer
Expand All @@ -130,6 +98,33 @@ const FilterTitlePane: FC<Props> = ({
restoreFilter={restoreFilter}
/>
</div>
<div
css={{
display: 'flex',
justifyContent: 'space-around',
alignItems: 'flex-start',
paddingTop: theme.gridUnit * 3,
}}
>
<Button
buttonSize="default"
buttonStyle="secondary"
icon={<Icons.Filter iconSize="m" />}
data-test="add-new-filter-button"
onClick={() => handleOnAdd(NativeFilterType.NativeFilter)}
>
{t('Add Filter')}
</Button>
<Button
buttonSize="default"
buttonStyle="secondary"
icon={<Icons.PicCenterOutlined iconSize="m" />}
data-test="add-new-divider-button"
onClick={() => handleOnAdd(NativeFilterType.Divider)}
>
{t('Add Divider')}
</Button>
</div>
</TabsContainer>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down

0 comments on commit 45815d8

Please sign in to comment.