From 259a50c468a7e7237eef91e398d6710dc4ea4940 Mon Sep 17 00:00:00 2001 From: Jillian Date: Sat, 24 Aug 2024 09:41:53 +0930 Subject: [PATCH] UI fixes for Sort Library Component [FC-0059] (#1222) 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 --- .../LibraryAuthoringPage.test.tsx | 111 +++++++++++------- src/search-manager/SearchManager.ts | 22 ++-- src/search-manager/SearchSortWidget.tsx | 65 +++++++--- src/search-manager/messages.ts | 7 +- 4 files changed, 129 insertions(+), 76 deletions(-) diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 3dbd737bc5..2481ce80e0 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -189,33 +189,33 @@ describe('', () => { axiosMock.onGet(getContentLibraryApiUrl(libraryData.id)).reply(200, libraryData); const { - getByRole, getByText, queryByText, findByText, findAllByText, + getByRole, getAllByText, getByText, queryByText, findByText, findAllByText, } = render(); - // 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(); @@ -224,7 +224,8 @@ describe('', () => { // 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(); }); @@ -239,10 +240,7 @@ describe('', () => { 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(); }); @@ -304,16 +302,13 @@ describe('', () => { 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(); @@ -396,15 +391,13 @@ describe('', () => { getByRole, getByText, queryByText, getAllByText, findAllByText, } = render(); - // 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(); @@ -416,7 +409,8 @@ describe('', () => { // 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(); @@ -424,7 +418,8 @@ describe('', () => { // 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(); }); @@ -438,15 +433,13 @@ describe('', () => { getByText, queryByText, getAllByText, findAllByText, } = render(); - // 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(); @@ -463,18 +456,25 @@ describe('', () => { fetchMock.post(searchEndpoint, returnEmptyResult, { overwriteRoutes: true }); const { - findByTitle, getAllByText, getByText, getByTitle, + findByTitle, getAllByText, getByRole, getByTitle, } = render(); 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), @@ -482,16 +482,23 @@ describe('', () => { 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'), @@ -500,8 +507,22 @@ describe('', () => { }); }); - // 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(), + }); + }); }); }); diff --git a/src/search-manager/SearchManager.ts b/src/search-manager/SearchManager.ts index 4db9e6f21b..d75a6cbdba 100644 --- a/src/search-manager/SearchManager.ts +++ b/src/search-manager/SearchManager.ts @@ -28,6 +28,7 @@ export interface SearchContextData { isFiltered: boolean; searchSortOrder: SearchSortOption; setSearchSortOrder: React.Dispatch>; + defaultSearchSortOrder: SearchSortOption; hits: ContentHit[]; totalHits: number; isFetching: boolean; @@ -65,14 +66,11 @@ function useStateWithUrlSearchParam( 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 }); @@ -95,9 +93,10 @@ 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( - defaultSortOption, + defaultSearchSortOrder, 'sort', (value: string) => Object.values(SearchSortOption).find((enumValue) => value === enumValue), (value: SearchSortOption) => value.toString(), @@ -105,7 +104,7 @@ export const SearchContextProvider: React.FC<{ // 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'); @@ -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: @@ -160,6 +157,7 @@ export const SearchContextProvider: React.FC<{ clearFilters, searchSortOrder, setSearchSortOrder, + defaultSearchSortOrder, closeSearchModal: props.closeSearchModal ?? (() => {}), hasError: hasConnectionError || result.isError, ...result, diff --git a/src/search-manager/SearchSortWidget.tsx b/src/search-manager/SearchSortWidget.tsx index 6885859b3c..01309845dd 100644 --- a/src/search-manager/SearchSortWidget.tsx +++ b/src/search-manager/SearchSortWidget.tsx @@ -9,47 +9,71 @@ import { useSearchContext } from './SearchManager'; export const SearchSortWidget: React.FC> = () => { const intl = useIntl(); + const { + searchSortOrder, + setSearchSortOrder, + defaultSearchSortOrder, + } = useSearchContext(); + const menuItems = useMemo( () => [ + { + id: 'search-sort-option-most-relevant', + name: intl.formatMessage(messages.searchSortMostRelevant), + value: SearchSortOption.RELEVANCE, + show: (defaultSearchSortOrder === SearchSortOption.RELEVANCE), + }, + { + id: 'search-sort-option-recently-modified', + name: intl.formatMessage(messages.searchSortRecentlyModified), + value: SearchSortOption.RECENTLY_MODIFIED, + show: true, + }, + { + id: 'search-sort-option-recently-published', + name: intl.formatMessage(messages.searchSortRecentlyPublished), + value: SearchSortOption.RECENTLY_PUBLISHED, + show: true, + }, { id: 'search-sort-option-title-az', name: intl.formatMessage(messages.searchSortTitleAZ), value: SearchSortOption.TITLE_AZ, + show: true, }, { id: 'search-sort-option-title-za', name: intl.formatMessage(messages.searchSortTitleZA), value: SearchSortOption.TITLE_ZA, + show: true, }, { id: 'search-sort-option-newest', name: intl.formatMessage(messages.searchSortNewest), value: SearchSortOption.NEWEST, + show: true, }, { id: 'search-sort-option-oldest', name: intl.formatMessage(messages.searchSortOldest), value: SearchSortOption.OLDEST, - }, - { - id: 'search-sort-option-recently-published', - name: intl.formatMessage(messages.searchSortRecentlyPublished), - value: SearchSortOption.RECENTLY_PUBLISHED, - }, - { - id: 'search-sort-option-recently-modified', - name: intl.formatMessage(messages.searchSortRecentlyModified), - value: SearchSortOption.RECENTLY_MODIFIED, + show: true, }, ], - [intl], + [intl, defaultSearchSortOrder], ); - const { searchSortOrder, setSearchSortOrder } = useSearchContext(); - const selectedSortOption = menuItems.find((menuItem) => menuItem.value === searchSortOrder); - const searchSortLabel = ( - selectedSortOption ? selectedSortOption.name : intl.formatMessage(messages.searchSortWidgetLabel) + const menuHeader = intl.formatMessage(messages.searchSortWidgetLabel); + const defaultSortOption = menuItems.find( + ({ value }) => (value === defaultSearchSortOrder), ); + const shownMenuItems = menuItems.filter(({ show }) => show); + + // Show the currently selected sort option as the toggle button label. + const selectedSortOption = shownMenuItems.find( + ({ value }) => (value === searchSortOrder), + ) ?? defaultSortOption; + const toggleLabel = selectedSortOption ? selectedSortOption.name : menuHeader; return ( @@ -62,13 +86,18 @@ export const SearchSortWidget: React.FC> = () => { size="sm" > - {searchSortLabel} +
{toggleLabel}
- {menuItems.map(({ id, name, value }) => ( + {menuHeader} + {shownMenuItems.map(({ id, name, value }) => ( setSearchSortOrder(value)} + onClick={() => { + // If the selected sort option was re-clicked, de-select it (reset to default) + const searchOrder = value === searchSortOrder ? defaultSearchSortOrder : value; + setSearchSortOrder(searchOrder); + }} > {name} {(value === searchSortOrder) && } diff --git a/src/search-manager/messages.ts b/src/search-manager/messages.ts index 8cd2e506ef..1fa3e229ea 100644 --- a/src/search-manager/messages.ts +++ b/src/search-manager/messages.ts @@ -132,7 +132,7 @@ const messages = defineMessages({ }, searchSortWidgetLabel: { id: 'course-authoring.course-search.searchSortWidget.label', - defaultMessage: 'Sort', + defaultMessage: 'Sort By', description: 'Label displayed to users when default sorting is used by the content search drop-down menu', }, searchSortWidgetAltTitle: { @@ -170,6 +170,11 @@ const messages = defineMessages({ defaultMessage: 'Recently Modified', description: 'Label for the content search sort drop-down which sorts by modified date, descending', }, + searchSortMostRelevant: { + id: 'course-authoring.course-search.searchSort.mostRelevant', + defaultMessage: 'Most Relevant', + description: 'Label for the content search sort drop-down which sorts keyword searches by relevance', + }, }); export default messages;