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.

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: a11y and performance issues with library authoring navigation [FC-0076] #1593

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
10 changes: 10 additions & 0 deletions src/search-modal/SearchModal.test.tsx
Original file line number Diff line number Diff line change
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',
});
const { getByRole } = render(<RootWrapper />);
expect(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
Loading