Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Sorting charts/dashboards makes the applied filters ineffective #27258

Merged
merged 1 commit into from
Feb 27, 2024
Merged

fix: Sorting charts/dashboards makes the applied filters ineffective #27258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,13 @@ describe('Charts list', () => {
orderAlphabetical();
cy.getBySel('styled-card').first().contains('% Rural');
});

it('should preserve other filters when sorting', () => {
cy.getBySel('styled-card').should('have.length', 25);
setFilter('Type', 'Big Number');
setFilter('Sort', 'Least recently modified');
cy.getBySel('styled-card').should('have.length', 3);
});
Comment on lines +176 to +182
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

});

describe('common actions', () => {
Expand Down
7 changes: 7 additions & 0 deletions superset-frontend/cypress-base/cypress/e2e/dashboard_list/list.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,13 @@ describe('Dashboards list', () => {
orderAlphabetical();
cy.getBySel('styled-card').first().contains('Supported Charts Dashboard');
});

it('should preserve other filters when sorting', () => {
cy.getBySel('styled-card').should('have.length', 5);
setFilter('Status', 'Published');
setFilter('Sort', 'Least recently modified');
cy.getBySel('styled-card').should('have.length', 3);
});
});

describe('common actions', () => {
Expand Down
18 changes: 9 additions & 9 deletions superset-frontend/src/components/ListView/CardSortSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { styled, t } from '@superset-ui/core';
import { Select } from 'src/components';
import { FormLabel } from 'src/components/Form';
import { SELECT_WIDTH } from './utils';
import { CardSortSelectOption, FetchDataConfig, SortColumn } from './types';
import { CardSortSelectOption, SortColumn } from './types';

const SortContainer = styled.div`
display: inline-flex;
Expand All @@ -32,22 +32,22 @@ const SortContainer = styled.div`
`;

interface CardViewSelectSortProps {
onChange: (conf: FetchDataConfig) => any;
onChange: (value: SortColumn[]) => void;
options: Array<CardSortSelectOption>;
initialSort?: SortColumn[];
pageIndex: number;
pageSize: number;
}

export const CardSortSelect = ({
initialSort,
onChange,
options,
pageIndex,
pageSize,
}: CardViewSelectSortProps) => {
const defaultSort =
(initialSort && options.find(({ id }) => id === initialSort[0].id)) ||
(initialSort &&
options.find(
({ id, desc }) =>
id === initialSort[0].id && desc === initialSort[0].desc,
)) ||
options[0];

const [value, setValue] = useState({
Expand All @@ -72,7 +72,7 @@ export const CardSortSelect = ({
desc: originalOption.desc,
},
];
onChange({ pageIndex, pageSize, sortBy, filters: [] });
onChange(sortBy);
}
};

Expand All @@ -82,7 +82,7 @@ export const CardSortSelect = ({
ariaLabel={t('Sort')}
header={<FormLabel>{t('Sort')}</FormLabel>}
labelInValue
onChange={(value: CardSortSelectOption) => handleOnChange(value)}
onChange={handleOnChange}
options={formattedOptions}
showSearch
value={value}
Expand Down
9 changes: 4 additions & 5 deletions superset-frontend/src/components/ListView/ListView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,11 @@ function ListView<T extends object = any>({
pageCount = 1,
gotoPage,
applyFilterValue,
setSortBy,
selectedFlatRows,
toggleAllRowsSelected,
setViewMode,
state: { pageIndex, pageSize, internalFilters, viewMode },
state: { pageIndex, pageSize, internalFilters, sortBy, viewMode },
query,
} = useListViewState({
bulkSelectColumnConfig,
Expand Down Expand Up @@ -350,11 +351,9 @@ function ListView<T extends object = any>({
)}
{viewMode === 'card' && cardSortSelectOptions && (
<CardSortSelect
initialSort={initialSort}
onChange={fetchData}
initialSort={sortBy}
onChange={(value: SortColumn[]) => setSortBy(value)}
options={cardSortSelectOptions}
pageIndex={pageIndex}
pageSize={pageSize}
/>
)}
</div>
Expand Down
4 changes: 1 addition & 3 deletions superset-frontend/src/components/ListView/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ export interface SortColumn {
desc?: boolean;
}

export type SortColumns = SortColumn[];

export interface SelectOption {
label: string;
value: any;
Expand Down Expand Up @@ -84,7 +82,7 @@ export interface FilterValue {
export interface FetchDataConfig {
pageIndex: number;
pageSize: number;
sortBy: SortColumns;
sortBy: SortColumn[];
Comment on lines -87 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason why this is being changed from SortColumns to SortColumn[] here? While on this topic, I personally don't like these types of pluralization aliases, as it makes it seem like it's something more than just an array of the singular type. But if we want to refrain from making these types of sweeping changes during the 4.0 hardening phase, I'd recommend leaving this as-is for now, and following up later with a PR that removes the plural type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I find following the "boy scout" approach is usually a good way of driving incremental change and reduces the pain of having to invest a chunk of time and effort into doing big bang changes. That's especially true if there are concerns over adequate test coverage for the "big bang" change, for example.

Though that's assuming there's already an agreed policy to remove aliases like these, otherwise the risk is different groups enacting different boy scout policies, which might be more of a concern in a large open source project like this one than I'm typically used to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some reason why this is being changed from SortColumns to SortColumn[] here?

I personally don't like these types of pluralization aliases, as it makes it seem like it's something more than just an array of the singular type.

The reason is exactly this 👆🏼

But if we want to refrain from making these types of sweeping changes during the 4.0 hardening phase, I'd recommend leaving this as-is for now, and following up later with a PR that removes the plural type.

This is an internal type used only by the ListView component.

filters: FilterValue[];
}

Expand Down
4 changes: 3 additions & 1 deletion superset-frontend/src/components/ListView/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ export function useListViewState({
query.sortColumn && query.sortOrder
? [{ id: query.sortColumn, desc: query.sortOrder === 'desc' }]
: initialSort,
[query.sortColumn, query.sortOrder],
[initialSort, query.sortColumn, query.sortOrder],
);

const initialState = {
Expand Down Expand Up @@ -256,6 +256,7 @@ export function useListViewState({
pageCount,
gotoPage,
setAllFilters,
setSortBy,
selectedFlatRows,
toggleAllRowsSelected,
state: { pageIndex, pageSize, sortBy, filters },
Expand Down Expand Up @@ -373,6 +374,7 @@ export function useListViewState({
rows,
selectedFlatRows,
setAllFilters,
setSortBy,
state: { pageIndex, pageSize, sortBy, filters, internalFilters, viewMode },
toggleAllRowsSelected,
applyFilterValue,
Expand Down
Loading