Skip to content

Commit

Permalink
UI fixes for Sort Library Component [FC-0059] (#1222)
Browse files Browse the repository at this point in the history 
fix: use "Recently Modified" as the default sort option

When search keyword(s) are entered, use "Most Relevant" as default.

Also

* Hides "Most Relevant" option if no keyword is entered.
* Re-orders the sort menu options
* Ensures the default sort option is not stored in the query string
* Shows the selected sort option on the drop-down toggle button
* Shows "Sort By" as a header inside the drop-down menu
  • Loading branch information
@pomegranited
pomegranited authored Aug 24, 2024
1 parent 3e0f7b5 commit 259a50c
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 76 deletions.
111 changes: 66 additions & 45 deletions src/library-authoring/LibraryAuthoringPage.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,33 +189,33 @@ describe('<LibraryAuthoringPage />', () => {
axiosMock.onGet(getContentLibraryApiUrl(libraryData.id)).reply(200, libraryData);

const {
getByRole, getByText, queryByText, findByText, findAllByText,
getByRole, getAllByText, getByText, queryByText, findByText, findAllByText,
} = render(<RootWrapper />);

// Ensure the search endpoint is called:
// Call 1: To fetch searchable/filterable/sortable library data
// Call 2: To fetch the recently modified components only
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });

expect(await findByText('Content library')).toBeInTheDocument();
expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();

expect(queryByText('You have not added any content to this library yet.')).not.toBeInTheDocument();

expect(getByText('Recently Modified')).toBeInTheDocument();
// "Recently Modified" header + sort shown
expect(getAllByText('Recently Modified').length).toEqual(2);
expect(getByText('Collections (0)')).toBeInTheDocument();
expect(getByText('Components (6)')).toBeInTheDocument();
expect((await findAllByText('Test HTML Block'))[0]).toBeInTheDocument();

// Navigate to the components tab
fireEvent.click(getByRole('tab', { name: 'Components' }));
expect(queryByText('Recently Modified')).not.toBeInTheDocument();
// "Recently Modified" default sort shown
expect(getAllByText('Recently Modified').length).toEqual(1);
expect(queryByText('Collections (0)')).not.toBeInTheDocument();
expect(queryByText('Components (6)')).not.toBeInTheDocument();

// Navigate to the collections tab
fireEvent.click(getByRole('tab', { name: 'Collections' }));
expect(queryByText('Recently Modified')).not.toBeInTheDocument();
// "Recently Modified" default sort shown
expect(getAllByText('Recently Modified').length).toEqual(1);
expect(queryByText('Collections (0)')).not.toBeInTheDocument();
expect(queryByText('Components (6)')).not.toBeInTheDocument();
expect(queryByText('There are 6 components in this library')).not.toBeInTheDocument();
Expand All @@ -224,7 +224,8 @@ describe('<LibraryAuthoringPage />', () => {
// Go back to Home tab
// This step is necessary to avoid the url change leak to other tests
fireEvent.click(getByRole('tab', { name: 'Home' }));
expect(getByText('Recently Modified')).toBeInTheDocument();
// "Recently Modified" header + sort shown
expect(getAllByText('Recently Modified').length).toEqual(2);
expect(getByText('Collections (0)')).toBeInTheDocument();
expect(getByText('Components (6)')).toBeInTheDocument();
});
Expand All @@ -239,10 +240,7 @@ describe('<LibraryAuthoringPage />', () => {
expect(await findByText('Content library')).toBeInTheDocument();
expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();

// Ensure the search endpoint is called:
// Call 1: To fetch searchable/filterable/sortable library data
// Call 2: To fetch the recently modified components only
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });

expect(getByText('You have not added any content to this library yet.')).toBeInTheDocument();
});
Expand Down Expand Up @@ -304,16 +302,13 @@ describe('<LibraryAuthoringPage />', () => {
expect(await findByText('Content library')).toBeInTheDocument();
expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();

// Ensure the search endpoint is called:
// Call 1: To fetch searchable/filterable/sortable library data
// Call 2: To fetch the recently modified components only
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });

fireEvent.change(getByRole('searchbox'), { target: { value: 'noresults' } });

// Ensure the search endpoint is called again, only once more since the recently modified call
// should not be impacted by the search
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(3, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });

expect(getByText('No matching components found in this library.')).toBeInTheDocument();

Expand Down Expand Up @@ -396,15 +391,13 @@ describe('<LibraryAuthoringPage />', () => {
getByRole, getByText, queryByText, getAllByText, findAllByText,
} = render(<RootWrapper />);

// Ensure the search endpoint is called:
// Call 1: To fetch searchable/filterable/sortable library data
// Call 2: To fetch the recently modified components only
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });

expect(getByText('Content library')).toBeInTheDocument();
expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();

await waitFor(() => { expect(getByText('Recently Modified')).toBeInTheDocument(); });
// "Recently Modified" header + sort shown
await waitFor(() => { expect(getAllByText('Recently Modified').length).toEqual(2); });
expect(getByText('Collections (0)')).toBeInTheDocument();
expect(getByText('Components (6)')).toBeInTheDocument();
expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();
Expand All @@ -416,15 +409,17 @@ describe('<LibraryAuthoringPage />', () => {

// Clicking on "View All" button should navigate to the Components tab
fireEvent.click(getByText('View All'));
expect(queryByText('Recently Modified')).not.toBeInTheDocument();
// "Recently Modified" default sort shown
expect(getAllByText('Recently Modified').length).toEqual(1);
expect(queryByText('Collections (0)')).not.toBeInTheDocument();
expect(queryByText('Components (6)')).not.toBeInTheDocument();
expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();

// Go back to Home tab
// This step is necessary to avoid the url change leak to other tests
fireEvent.click(getByRole('tab', { name: 'Home' }));
expect(getByText('Recently Modified')).toBeInTheDocument();
// "Recently Modified" header + sort shown
expect(getAllByText('Recently Modified').length).toEqual(2);
expect(getByText('Collections (0)')).toBeInTheDocument();
expect(getByText('Components (6)')).toBeInTheDocument();
});
Expand All @@ -438,15 +433,13 @@ describe('<LibraryAuthoringPage />', () => {
getByText, queryByText, getAllByText, findAllByText,
} = render(<RootWrapper />);

// Ensure the search endpoint is called:
// Call 1: To fetch searchable/filterable/sortable library data
// Call 2: To fetch the recently modified components only
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(2, searchEndpoint, 'post'); });
await waitFor(() => { expect(fetchMock).toHaveFetchedTimes(1, searchEndpoint, 'post'); });

expect(getByText('Content library')).toBeInTheDocument();
expect((await findAllByText(libraryData.title))[0]).toBeInTheDocument();

await waitFor(() => { expect(getByText('Recently Modified')).toBeInTheDocument(); });
// "Recently Modified" header + sort shown
await waitFor(() => { expect(getAllByText('Recently Modified').length).toEqual(2); });
expect(getByText('Collections (0)')).toBeInTheDocument();
expect(getByText('Components (2)')).toBeInTheDocument();
expect(getAllByText('Test HTML Block')[0]).toBeInTheDocument();
Expand All @@ -463,35 +456,49 @@ describe('<LibraryAuthoringPage />', () => {
fetchMock.post(searchEndpoint, returnEmptyResult, { overwriteRoutes: true });

const {
findByTitle, getAllByText, getByText, getByTitle,
findByTitle, getAllByText, getByRole, getByTitle,
} = render(<RootWrapper />);

expect(await findByTitle('Sort search results')).toBeInTheDocument();

const testSortOption = (async (optionText, sortBy) => {
if (optionText) {
fireEvent.click(getByTitle('Sort search results'));
fireEvent.click(getByText(optionText));
}
const testSortOption = (async (optionText, sortBy, isDefault) => {
// Open the drop-down menu
fireEvent.click(getByTitle('Sort search results'));

// Click the option with the given text
// Since the sort drop-down also shows the selected sort
// option in its toggle button, we need to make sure we're
// clicking on the last one found.
const options = getAllByText(optionText);
expect(options.length).toBeGreaterThan(0);
fireEvent.click(options[options.length - 1]);

// Did the search happen with the expected sort option?
const bodyText = sortBy ? `"sort":["${sortBy}"]` : '"sort":[]';
const searchText = sortBy ? `?sort=${encodeURIComponent(sortBy)}` : '';
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.stringContaining(bodyText),
method: 'POST',
headers: expect.anything(),
});
});

// Is the sort option stored in the query string?
const searchText = isDefault ? '' : `?sort=${encodeURIComponent(sortBy)}`;
expect(window.location.search).toEqual(searchText);

// Is the selected sort option shown in the toggle button (if not default)
// as well as in the drop-down menu?
expect(getAllByText(optionText).length).toEqual(isDefault ? 1 : 2);
});

await testSortOption('Title, A-Z', 'display_name:asc');
await testSortOption('Title, Z-A', 'display_name:desc');
await testSortOption('Newest', 'created:desc');
await testSortOption('Oldest', 'created:asc');
await testSortOption('Title, A-Z', 'display_name:asc', false);
await testSortOption('Title, Z-A', 'display_name:desc', false);
await testSortOption('Newest', 'created:desc', false);
await testSortOption('Oldest', 'created:asc', false);

// Sorting by Recently Published also excludes unpublished components
await testSortOption('Recently Published', 'last_published:desc');
await testSortOption('Recently Published', 'last_published:desc', false);
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.stringContaining('last_published IS NOT NULL'),
Expand All @@ -500,8 +507,22 @@ describe('<LibraryAuthoringPage />', () => {
});
});

// Clearing filters clears the url search param and uses default sort
fireEvent.click(getAllByText('Clear Filters')[0]);
await testSortOption('', '');
// Re-selecting the previous sort option resets sort to default "Recently Modified"
await testSortOption('Recently Published', 'modified:desc', true);
expect(getAllByText('Recently Modified').length).toEqual(2);

// Enter a keyword into the search box
const searchBox = getByRole('searchbox');
fireEvent.change(searchBox, { target: { value: 'words to find' } });

// Default sort option changes to "Most Relevant"
expect(getAllByText('Most Relevant').length).toEqual(2);
await waitFor(() => {
expect(fetchMock).toHaveBeenLastCalledWith(searchEndpoint, {
body: expect.stringContaining('"sort":[]'),
method: 'POST',
headers: expect.anything(),
});
});
});
});
22 changes: 10 additions & 12 deletions src/search-manager/SearchManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface SearchContextData {
isFiltered: boolean;
searchSortOrder: SearchSortOption;
setSearchSortOrder: React.Dispatch<React.SetStateAction<SearchSortOption>>;
defaultSearchSortOrder: SearchSortOption;
hits: ContentHit[];
totalHits: number;
isFetching: boolean;
Expand Down Expand Up @@ -65,14 +66,11 @@ function useStateWithUrlSearchParam<Type>(
setSearchParams((prevParams) => {
const paramValue: string = toString(value) ?? '';
const newSearchParams = new URLSearchParams(prevParams);
if (paramValue) {
newSearchParams.set(paramName, paramValue);
} else {
// If no paramValue, remove it from the search params, so
// we don't get dangling parameter values like ?paramName=
// Another way to decide this would be to check value === defaultValue,
// and ensure that default values are never stored in the search string.
// If using the default paramValue, remove it from the search params.
if (paramValue === defaultValue) {
newSearchParams.delete(paramName);
} else {
newSearchParams.set(paramName, paramValue);
}
return newSearchParams;
}, { replace: true });
Expand All @@ -95,17 +93,18 @@ export const SearchContextProvider: React.FC<{

// The search sort order can be set via the query string
// E.g. ?sort=display_name:desc maps to SearchSortOption.TITLE_ZA.
const defaultSortOption = SearchSortOption.RELEVANCE;
// Default sort by Most Relevant if there's search keyword(s), else by Recently Modified.
const defaultSearchSortOrder = searchKeywords ? SearchSortOption.RELEVANCE : SearchSortOption.RECENTLY_MODIFIED;
const [searchSortOrder, setSearchSortOrder] = useStateWithUrlSearchParam<SearchSortOption>(
defaultSortOption,
defaultSearchSortOrder,
'sort',
(value: string) => Object.values(SearchSortOption).find((enumValue) => value === enumValue),
(value: SearchSortOption) => value.toString(),
);
// SearchSortOption.RELEVANCE is special, it means "no custom sorting", so we
// send it to useContentSearchResults as an empty array.
const searchSortOrderToUse = overrideSearchSortOrder ?? searchSortOrder;
const sort: SearchSortOption[] = (searchSortOrderToUse === defaultSortOption ? [] : [searchSortOrderToUse]);
const sort: SearchSortOption[] = (searchSortOrderToUse === SearchSortOption.RELEVANCE ? [] : [searchSortOrderToUse]);
// Selecting SearchSortOption.RECENTLY_PUBLISHED also excludes unpublished components.
if (searchSortOrderToUse === SearchSortOption.RECENTLY_PUBLISHED) {
extraFilter.push('last_published IS NOT NULL');
Expand All @@ -114,13 +113,11 @@ export const SearchContextProvider: React.FC<{
const canClearFilters = (
blockTypesFilter.length > 0
|| tagsFilter.length > 0
|| searchSortOrderToUse !== defaultSortOption
);
const isFiltered = canClearFilters || (searchKeywords !== '');
const clearFilters = React.useCallback(() => {
setBlockTypesFilter([]);
setTagsFilter([]);
setSearchSortOrder(defaultSortOption);
}, []);

// Initialize a connection to Meilisearch:
Expand Down Expand Up @@ -160,6 +157,7 @@ export const SearchContextProvider: React.FC<{
clearFilters,
searchSortOrder,
setSearchSortOrder,
defaultSearchSortOrder,
closeSearchModal: props.closeSearchModal ?? (() => {}),
hasError: hasConnectionError || result.isError,
...result,
Expand Down
Loading

0 comments on commit 259a50c

Please sign in to comment.