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

Jump to bottom

Feature/improve sorting ux #1541 #1578

Merged
merged 25 commits into from
Oct 25, 2023
Merged

Feature/improve sorting ux #1541 #1578

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
70a8178
Added shift+click sorting #1541
kaperoo Sep 1, 2023
d1294dd
Code cleanup #1541
kaperoo Sep 1, 2023
88d2702
Sorting unit test fix #1541
kaperoo Sep 1, 2023
a28a6bd
Shift-click sorting for download tables #1541
kaperoo Sep 4, 2023
e03740d
Fixed sorting in card view #1541
kaperoo Sep 4, 2023
374c857
changed the way shift is passed to onSort #1541
kaperoo Sep 5, 2023
0586bd4
Fix issues with download e2e tests #1541
kaperoo Sep 5, 2023
ca20c22
changed the way shiftDown is passed in cardview #1541
kaperoo Sep 5, 2023
52d0395
fix to the single column sort test in admin download #1541
kaperoo Sep 5, 2023
72be066
Fixed dataview e2e tests #1541
kaperoo Sep 6, 2023
e94c94a
Fix dataview e2e tests 2 #1541
kaperoo Sep 6, 2023
76f26b8
Improved code coverage #1541
kaperoo Sep 8, 2023
f86c192
Merge branch 'develop' into feature/improve-sorting-ux-#1541
kaperoo Sep 11, 2023
79e4336
Adjusted + added e2e tests for multiple column sorting #1541
kaperoo Sep 11, 2023
77f3972
fix e2e test for adminDownloadDtatus #1541
kaperoo Sep 12, 2023
c6bd356
Merge branch 'develop' into feature/improve-sorting-ux-#1541
kaperoo Sep 12, 2023
be9ef8b
Reduce code repetition
kaperoo Sep 14, 2023
c9162f3
add hover tooltips to sort indicators
kaperoo Oct 11, 2023
7553763
Hopefully fixed failing tests
kaperoo Oct 12, 2023
7492fb3
updated snapshots
kaperoo Oct 12, 2023
95cf7fc
fix failing e2e test
kaperoo Oct 12, 2023
b6bca39
#1541 - use skipHover to avoid triggering sort tooltips in long tests
louise-davies Oct 12, 2023
0043575
Merge branch 'feature/improve-sorting-ux-#1541' of https://github.com…
louise-davies Oct 12, 2023
7fe8811
use skipHover to fix another test #1541
kaperoo Oct 13, 2023
848d799
#1541 - add delay null to downloadStatusTable userEvent
louise-davies Oct 13, 2023
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
42 changes: 42 additions & 0 deletions packages/datagateway-common/src/api/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,7 @@ describe('generic api functions', () => {
afterEach(() => {
jest.restoreAllMocks();
jest.resetModules();
window.history.pushState({}, 'Test', '/');
});

describe('useSort', () => {
Expand Down Expand Up @@ -398,6 +399,47 @@ describe('generic api functions', () => {
search: '?',
});
});

it('returns callback that, when called without shift modifier, replaces sort with the new one', () => {
window.history.pushState(
{},
'Test',
'?sort=%7B%22name%22%3A%22asc%22%7D'
);
const { result } = renderHook(() => useSort(), {
wrapper,
});

act(() => {
result.current('title', 'asc', 'push', false);
});

expect(pushSpy).toHaveBeenCalledWith({
search: `?sort=${encodeURIComponent('{"title":"asc"}')}`,
});
});

it('returns callback that, when called with shift modifier, appends new sort to the existing one', () => {
window.history.pushState(
{},
'Test',
'?sort=%7B%22name%22%3A%22asc%22%7D'
);

const { result } = renderHook(() => useSort(), {
wrapper,
});

act(() => {
result.current('title', 'asc', 'push', true);
});

console.log(history.location.search);

expect(pushSpy).toHaveBeenCalledWith({
search: `?sort=${encodeURIComponent('{"name":"asc","title":"asc"}')}`,
});
});
});

describe('usePushFilter', () => {
Expand Down
27 changes: 18 additions & 9 deletions packages/datagateway-common/src/api/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,25 +260,34 @@ export const getApiParams = (
export const useSort = (): ((
sortKey: string,
order: Order | null,
updateMethod: UpdateMethod
updateMethod: UpdateMethod,
shiftDown?: boolean
) => void) => {
const { push, replace } = useHistory();

return React.useCallback(
(
sortKey: string,
order: Order | null,
updateMethod: UpdateMethod
updateMethod: UpdateMethod,
shiftDown?: boolean
): void => {
let query = parseSearchToQuery(window.location.search);
if (order !== null) {
query = {
...query,
sort: {
...query.sort,
[sortKey]: order,
},
};
query = shiftDown
? {
...query,
sort: {
...query.sort,
[sortKey]: order,
},
}
: {
...query,
sort: {
[sortKey]: order,
},
};
} else {
// if order is null, user no longer wants to sort by that column so remove column from sort state
const { [sortKey]: order, ...rest } = query.sort;
Expand Down
66 changes: 56 additions & 10 deletions packages/datagateway-common/src/card/cardView.component.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe('Card View', () => {
await user.click(
await screen.findByRole('button', { name: 'Sort by TITLE' })
);
expect(onSort).toHaveBeenNthCalledWith(1, 'title', 'asc', 'push');
expect(onSort).toHaveBeenNthCalledWith(1, 'title', 'asc', 'push', false);

updatedProps = {
...updatedProps,
Expand All @@ -240,7 +240,7 @@ describe('Card View', () => {
name: 'Sort by TITLE, current direction ascending',
})
);
expect(onSort).toHaveBeenNthCalledWith(2, 'title', 'desc', 'push');
expect(onSort).toHaveBeenNthCalledWith(2, 'title', 'desc', 'push', false);

updatedProps = {
...updatedProps,
Expand All @@ -253,7 +253,7 @@ describe('Card View', () => {
name: 'Sort by TITLE, current direction descending',
})
);
expect(onSort).toHaveBeenNthCalledWith(3, 'title', null, 'push');
expect(onSort).toHaveBeenNthCalledWith(3, 'title', null, 'push', false);
});

it('default sort applied correctly', () => {
Expand All @@ -272,9 +272,9 @@ describe('Card View', () => {
};
render(<CardView {...updatedProps} />);

expect(onSort).toHaveBeenCalledWith('title', 'asc', 'replace');
expect(onSort).toHaveBeenCalledWith('name', 'desc', 'replace');
expect(onSort).toHaveBeenCalledWith('test', 'asc', 'replace');
expect(onSort).toHaveBeenCalledWith('title', 'asc', 'replace', false);
expect(onSort).toHaveBeenCalledWith('name', 'desc', 'replace', false);
expect(onSort).toHaveBeenCalledWith('test', 'asc', 'replace', false);
});

it('can sort by description with label', async () => {
Expand All @@ -290,7 +290,7 @@ describe('Card View', () => {
await user.click(
await screen.findByRole('button', { name: 'Sort by NAME' })
);
expect(onSort).toHaveBeenCalledWith('name', 'asc', 'push');
expect(onSort).toHaveBeenCalledWith('name', 'asc', 'push', false);
});

it('can sort by description without label', async () => {
Expand All @@ -306,7 +306,7 @@ describe('Card View', () => {
await user.click(
await screen.findByRole('button', { name: 'Sort by NAME' })
);
expect(onSort).toHaveBeenCalledWith('name', 'asc', 'push');
expect(onSort).toHaveBeenCalledWith('name', 'asc', 'push', false);
});

it('page changed when sort applied', async () => {
Expand All @@ -318,7 +318,7 @@ describe('Card View', () => {
await screen.findByRole('button', { name: 'Sort by TITLE' })
);

expect(onSort).toHaveBeenCalledWith('title', 'asc', 'push');
expect(onSort).toHaveBeenCalledWith('title', 'asc', 'push', false);
expect(onPageChange).toHaveBeenCalledWith(1);
});

Expand Down Expand Up @@ -377,7 +377,53 @@ describe('Card View', () => {
await user.click(
await screen.findByRole('button', { name: 'Sort by VISITID' })
);
expect(onSort).toHaveBeenCalledWith('visitId', 'asc', 'push');
expect(onSort).toHaveBeenCalledWith('visitId', 'asc', 'push', false);
});

it('calls onSort with correct parameters when shift key is pressed', async () => {
const updatedProps = {
...props,
title: { dataKey: 'title', disableSort: true },
description: { dataKey: 'name', disableSort: true },
information: [
{ dataKey: 'visitId' },
{
dataKey: 'name',
label: 'Name',
content: (entity: Entity) => entity.name,
},
],
page: 1,
};
render(<CardView {...updatedProps} />);

// Click with shift to sort ascending
await user.keyboard('{Shift>}');
await user.click(
await screen.findByRole('button', { name: 'Sort by VISITID' })
);
await user.keyboard('{/Shift}');

expect(onSort).toHaveBeenCalledWith('visitId', 'asc', 'push', true);

await user.click(
await screen.findByRole('button', { name: 'Sort by NAME' })
);

expect(onSort).toHaveBeenCalledWith('name', 'asc', 'push', false);
});

it('displays correct icon when no sort applied', async () => {
render(<CardView {...props} />);

const cards = await screen.findAllByRole('button', { name: /Sort by/ });
expect(
within(cards[0]).getByTestId(`ArrowDownwardIcon`)
).toBeInTheDocument();

await user.keyboard('{Shift>}');
expect(within(cards[0]).getByTestId(`AddIcon`)).toBeInTheDocument();
await user.keyboard('{/Shift}');
});

it('information displays with content that has no tooltip', async () => {
Expand Down
76 changes: 62 additions & 14 deletions packages/datagateway-common/src/card/cardView.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import React from 'react';
import { useTranslation } from 'react-i18next';
import AdvancedFilter from './advancedFilter.component';
import EntityCard, { EntityImageDetails } from './entityCard.component';
import AddIcon from '@mui/icons-material/Add';

const SelectedChips = styled('ul')(({ theme }) => ({
display: 'inline-flex',
Expand Down Expand Up @@ -85,7 +86,8 @@ export interface CardViewProps {
onSort: (
sort: string,
order: Order | null,
updateMethod: UpdateMethod
updateMethod: UpdateMethod,
shiftDown?: boolean
) => void;

// Props to get title, description of the card
Expand Down Expand Up @@ -189,6 +191,30 @@ const CardView = (props: CardViewProps): React.ReactElement => {
sort,
} = props;

const [shiftDown, setShiftDown] = React.useState(false);
// add event listener to listen for shift key being pressed
React.useEffect(() => {
const handleKeyDown = (event: KeyboardEvent): void => {
if (event.key === 'Shift') {
setShiftDown(true);
}
};

const handleKeyUp = (event: KeyboardEvent): void => {
if (event.key === 'Shift') {
setShiftDown(false);
}
};

document.addEventListener('keydown', handleKeyDown);
document.addEventListener('keyup', handleKeyUp);

return (): void => {
document.removeEventListener('keydown', handleKeyDown);
document.removeEventListener('keyup', handleKeyUp);
};
}, []);

// Results options (by default it is 10, 20 and 30).
const resOptions = React.useMemo(
() =>
Expand Down Expand Up @@ -233,20 +259,20 @@ const CardView = (props: CardViewProps): React.ReactElement => {
//defaultSort has been provided
React.useEffect(() => {
if (title.defaultSort !== undefined && sort[title.dataKey] === undefined)
onSort(title.dataKey, title.defaultSort, 'replace');
onSort(title.dataKey, title.defaultSort, 'replace', false);
Copy link
Member

Choose a reason for hiding this comment

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

Minor comment, but since you specified shiftDown?: boolean as optional, you could leave out explicitly specifying false here. Would have meant less changes in the tests - but no point removing it now, just an observation

if (
description &&
description.defaultSort !== undefined &&
sort[description.dataKey] === undefined
)
onSort(description.dataKey, description.defaultSort, 'replace');
onSort(description.dataKey, description.defaultSort, 'replace', false);
if (information) {
information.forEach((element: CardViewDetails) => {
if (
element.defaultSort !== undefined &&
sort[element.dataKey] === undefined
)
onSort(element.dataKey, element.defaultSort, 'replace');
onSort(element.dataKey, element.defaultSort, 'replace', false);
});
}
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -593,11 +619,12 @@ const CardView = (props: CardViewProps): React.ReactElement => {
<ListItem
key={i}
button
onClick={() => {
onClick={(event) => {
onSort(
s.dataKey,
nextSortDirection(s.dataKey),
'push'
'push',
event.shiftKey
);
if (page !== 1) {
onPageChange(1);
Expand All @@ -615,14 +642,35 @@ const CardView = (props: CardViewProps): React.ReactElement => {
>
<ListItemText primary={s.label} />
<ListItemIcon>
<TableSortLabel
active={s.dataKey in sort}
direction={sort[s.dataKey]}
// Set tabindex to -1 to prevent button focus
tabIndex={-1}
>
{s.dataKey in sort && sort[s.dataKey]}
</TableSortLabel>
{s.dataKey in sort ? (
<TableSortLabel
active={true}
direction={sort[s.dataKey]}
// Set tabindex to -1 to prevent button focus
tabIndex={-1}
>
{s.dataKey in sort && sort[s.dataKey]}
</TableSortLabel>
) : shiftDown ? (
<TableSortLabel
active={true}
direction={sort[s.dataKey]}
// Set tabindex to -1 to prevent button focus
tabIndex={-1}
IconComponent={AddIcon}
>
{s.dataKey in sort && sort[s.dataKey]}
</TableSortLabel>
Copy link
Member

Choose a reason for hiding this comment

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

You could simply & reduce code repetition here by removing the ternary checks and changing TableSortLabel to:

<TableSortLabel
   active={s.dataKey in sort}
   direction={sort[s.dataKey]}
   // Set tabindex to -1 to prevent button focus
   tabIndex={-1}
   IconComponent={!(s.dataKey in sort) && shiftDown ? AddIcon : undefined}
>
   {s.dataKey in sort && sort[s.dataKey]}
</TableSortLabel>

) : (
<TableSortLabel
active={false}
direction={sort[s.dataKey]}
// Set tabindex to -1 to prevent button focus
tabIndex={-1}
>
{s.dataKey in sort && sort[s.dataKey]}
</TableSortLabel>
)}
</ListItemIcon>
</ListItem>
))}
Expand Down
Loading
Loading