Skip to content

Commit

Permalink
fix: a11y and performance issues with library authoring navigation [F…
Browse files Browse the repository at this point in the history
…C-0076] (#1593)

Fixes some a11y and performance issues for Library Authors

**Accessibility**

* When navigating between the various Library Authoring pages, the search keyword box should not be auto-focused, so focus will follow the user's click/tab events and keep that continuity.
The course content search modal behavior is unchanged -- loading this modal auto-focuses the search input box as one would expect.

**Performance**

* Navigating between the various Library Authoring pages was causing a full re-mount of the `<LibraryLayout/>` component
* React query's `staleTime` option is used to control how long data is considered fresh in both queries and routes. By default, the `staleTime` for queries is set to 0 milliseconds, meaning that the data will always be considered stale and will be refetched whenever the query is mounted.
  • Loading branch information
pomegranited authored Jan 22, 2025
1 parent 13b2ed5 commit 654c06b
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 14 deletions.
8 changes: 7 additions & 1 deletion src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,13 @@ import { ToastProvider } from './generic/toast-context';
import 'react-datepicker/dist/react-datepicker.css';
import './index.scss';

const queryClient = new QueryClient();
const queryClient = new QueryClient({
defaultOptions: {
queries: {
staleTime: 60 * 60_000, // If cache is up to one hour old, no need to re-fetch
},
},
});

const App = () => {
useEffect(() => {
Expand Down
4 changes: 4 additions & 0 deletions src/library-authoring/LibraryAuthoringPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ describe('<LibraryAuthoringPage />', () => {
expect(screen.getAllByText('Recently Modified').length).toEqual(1);
expect((await screen.findAllByText('Introduction to Testing'))[0]).toBeInTheDocument();

// Search box should not have focus on page load
const searchBox = screen.getByRole('searchbox');
expect(searchBox).not.toHaveFocus();

// Navigate to the components tab
fireEvent.click(screen.getByRole('tab', { name: 'Components' }));
// "Recently Modified" default sort shown
Expand Down
18 changes: 11 additions & 7 deletions src/library-authoring/LibraryLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { useCallback } from 'react';
import {
Route,
Routes,
useMatch,
useParams,
useLocation,
type PathMatch,
} from 'react-router-dom';

import { ROUTES } from './routes';
import { BASE_ROUTE, ROUTES } from './routes';
import LibraryAuthoringPage from './LibraryAuthoringPage';
import { LibraryProvider } from './common/context/LibraryContext';
import { SidebarProvider } from './common/context/SidebarContext';
Expand All @@ -23,12 +24,15 @@ const LibraryLayout = () => {
throw new Error('Error: route is missing libraryId.');
}

const location = useLocation();
// The top-level route is `${BASE_ROUTE}/*`, so match will always be non-null.
const match = useMatch(`${BASE_ROUTE}${ROUTES.COLLECTION}`) as PathMatch<'libraryId' | 'collectionId'> | null;
const collectionId = match?.params.collectionId;

const context = useCallback((childPage) => (
<LibraryProvider
/** We need to pass the pathname as key to the LibraryProvider to force a
* re-render when we navigate to a new path or page. */
key={location.pathname}
/** We need to pass the collectionId as key to the LibraryProvider to force a re-render
* when we navigate to a collection page. */
key={collectionId}
libraryId={libraryId}
/** The component picker modal to use. We need to pass it as a reference instead of
* directly importing it to avoid the import cycle:
Expand All @@ -44,7 +48,7 @@ const LibraryLayout = () => {
</>
</SidebarProvider>
</LibraryProvider>
), [location.pathname]);
), [collectionId]);

return (
<Routes>
Expand Down
8 changes: 6 additions & 2 deletions src/search-manager/SearchKeywordsField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import { useSearchContext } from './SearchManager';
/**
* The "main" input field where users type in search keywords. The search happens as they type (no need to press enter).
*/
const SearchKeywordsField: React.FC<{ className?: string, placeholder?: string }> = (props) => {
const SearchKeywordsField: React.FC<{
className?: string,
placeholder?: string,
autoFocus?: boolean,
}> = (props) => {
const intl = useIntl();
const { searchKeywords, setSearchKeywords, usageKey } = useSearchContext();
const defaultPlaceholder = usageKey ? messages.clearUsageKeyToSearch : messages.inputPlaceholder;
Expand All @@ -24,7 +28,7 @@ const SearchKeywordsField: React.FC<{ className?: string, placeholder?: string }
>
<SearchField.Label />
<SearchField.Input
autoFocus
autoFocus={Boolean(props.autoFocus)}
placeholder={placeholder}
/>
<SearchField.ClearButton />
Expand Down
12 changes: 11 additions & 1 deletion src/search-modal/SearchModal.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';
import { IntlProvider } from '@edx/frontend-platform/i18n';
import { AppProvider } from '@edx/frontend-platform/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';
import type { Store } from 'redux';
import MockAdapter from 'axios-mock-adapter';

Expand Down Expand Up @@ -73,4 +73,14 @@ describe('<SearchModal />', () => {
const { findByText } = render(<RootWrapper />);
expect(await findByText('An error occurred. Unable to load search results.')).toBeInTheDocument();
});

it('should set focus on the search input box when loaded in the modal', async () => {
axiosMock.onGet(getContentSearchConfigUrl()).replyOnce(200, {
url: 'https://meilisearch.example.com',
index: 'test-index',
apiKey: 'test-api-key',
});
render(<RootWrapper />);
expect(screen.getByRole('searchbox')).toHaveFocus();
});
});
6 changes: 5 additions & 1 deletion src/search-modal/SearchModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ const SearchModal: React.FC<{ courseId?: string, isOpen: boolean, onClose: () =>
isFullscreenOnMobile
className="courseware-search-modal"
>
<SearchUI courseId={courseId} closeSearchModal={props.onClose} />
<SearchUI
courseId={courseId}
closeSearchModal={props.onClose}
autoFocus
/>
</ModalDialog>
);
};
Expand Down
11 changes: 9 additions & 2 deletions src/search-modal/SearchUI.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ import EmptyStates from './EmptyStates';
import SearchResults from './SearchResults';
import messages from './messages';

const SearchUI: React.FC<{ courseId?: string, closeSearchModal?: () => void }> = (props) => {
const SearchUI: React.FC<{
courseId?: string,
autoFocus?: boolean,
closeSearchModal?: () => void,
}> = (props) => {
const hasCourseId = Boolean(props.courseId);
const [searchThisCourseEnabled, setSearchThisCourse] = React.useState(hasCourseId);
const switchToThisCourse = React.useCallback(() => setSearchThisCourse(true), []);
Expand All @@ -39,7 +43,10 @@ const SearchUI: React.FC<{ courseId?: string, closeSearchModal?: () => void }> =
<ModalDialog.Header style={{ zIndex: 9 }} className="border-bottom">
<ModalDialog.Title><FormattedMessage {...messages.title} /></ModalDialog.Title>
<div className="d-flex mt-3">
<SearchKeywordsField className="flex-grow-1 mr-2" />
<SearchKeywordsField
className="flex-grow-1 mr-2"
autoFocus={props.autoFocus}
/>
<SelectMenu variant="primary">
<MenuItem
onClick={switchToThisCourse}
Expand Down

0 comments on commit 654c06b

Please sign in to comment.