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

feat: shareable URLs for library components and searches [FC-0076] #1575

Merged
merged 19 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
aa25827
fix: use generatePath in testUtils
pomegranited Dec 18, 2024
a4728e1
fix: test uses contentId=undefined, so made this param optional in th…
pomegranited Dec 19, 2024
a6465cb
Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…
pomegranited Dec 19, 2024
ce4c27e
feat: adds sharable URLs for library components/collections
pomegranited Dec 12, 2024
fc2f467
feat: store search keywords in query string
pomegranited Dec 19, 2024
f6b46c4
feat: store sidebar tab and additional action in query string
pomegranited Dec 19, 2024
37370d2
feat: store selected tags in the URL search string
pomegranited Dec 20, 2024
1736942
refactor: use one typesFilter state in SearchManager
pomegranited Dec 20, 2024
a3ee610
docs: adds more comments to the library authoring routes
pomegranited Dec 20, 2024
9d3e049
Merge remote-tracking branch 'origin/master' into jill/fal-3984-shara…
pomegranited Dec 30, 2024
5054020
fix: clicking "Library Info" navigates to the Library URL
pomegranited Dec 30, 2024
88dfb0e
fix: use sidebarAction instead of managing separate state
pomegranited Dec 30, 2024
9ae67f5
refactor: use getActiveKey to initialize the tab state
pomegranited Dec 30, 2024
2f28e3b
refactor: allow Type or Type[] values in useStateUrlSearchParams
pomegranited Dec 30, 2024
b4c70ca
fix: disable filter by type on collections tab
pomegranited Jan 1, 2025
ced360e
test: fixes broken tests
pomegranited Jan 1, 2025
d060113
test: clear search filters in between tests
pomegranited Jan 1, 2025
4c1855d
test: split long-running "can filter by capa problem type" test into two
pomegranited Jan 1, 2025
39fd745
test: adds tests for library authoring routes.ts
pomegranited Jan 10, 2025
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
2 changes: 1 addition & 1 deletion src/content-tags-drawer/ContentTagsDrawer.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
} from './data/api.mocks';
import { getContentTaxonomyTagsApiUrl } from './data/api';

const path = '/content/:contentId/*';
const path = '/content/:contentId?/*';
const mockOnClose = jest.fn();
const mockSetBlockingSheet = jest.fn();
const mockNavigate = jest.fn();
Expand Down
51 changes: 49 additions & 2 deletions src/hooks.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { useEffect, useState } from 'react';
import {
type Dispatch,
type SetStateAction,
useCallback,
useEffect,
useState,
} from 'react';
import { history } from '@edx/frontend-platform';
import { useLocation } from 'react-router-dom';
import { useLocation, useSearchParams } from 'react-router-dom';

export const useScrollToHashElement = ({ isLoading }: { isLoading: boolean }) => {
const [elementWithHash, setElementWithHash] = useState<string | null>(null);
Expand Down Expand Up @@ -77,3 +83,44 @@ export const useLoadOnScroll = (
return () => { };
}, [hasNextPage, isFetchingNextPage, fetchNextPage]);
};

/**
* Hook which stores state variables in the URL search parameters.
*
* It wraps useState with functions that get/set a query string
* search parameter when returning/setting the state variable.
*
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
* @param defaultValue: Type
* Returned when no valid value is found in the url search parameter.
* @param paramName: name of the url search parameter to store this value in.
* @param fromString: returns the Type equivalent of the given string value,
* or undefined if the value is invalid.
* @param toString: returns the string equivalent of the given Type value.
* Return defaultValue to clear the url search paramName.
*/
export function useStateWithUrlSearchParam<Type>(
defaultValue: Type,
paramName: string,
fromString: (value: string | null) => Type | undefined,
toString: (value: Type) => string | undefined,
): [value: Type, setter: Dispatch<SetStateAction<Type>>] {
const [searchParams, setSearchParams] = useSearchParams();
const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue;
// Function to update the url search parameter
const returnSetter: Dispatch<SetStateAction<Type>> = useCallback((value: Type) => {
setSearchParams((prevParams) => {
const paramValue: string = toString(value) ?? '';
const newSearchParams = new URLSearchParams(prevParams);
// 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 });
}, [setSearchParams]);

// Return the computed value and wrapped set state function
return [returnValue, returnSetter];
}
12 changes: 4 additions & 8 deletions src/library-authoring/LibraryAuthoringPage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -431,27 +431,23 @@ describe('<LibraryAuthoringPage />', () => {
// Click on the first collection
fireEvent.click((await screen.findByText('Collection 1')));

const sidebar = screen.getByTestId('library-sidebar');

const { getByRole } = within(sidebar);

// Click on the Details tab
fireEvent.click(getByRole('tab', { name: 'Details' }));
fireEvent.click(screen.getByRole('tab', { name: 'Details' }));

// Change to a component
fireEvent.click((await screen.findAllByText('Introduction to Testing'))[0]);

// Check that the Details tab is still selected
expect(getByRole('tab', { name: 'Details' })).toHaveAttribute('aria-selected', 'true');
expect(screen.getByRole('tab', { name: 'Details' })).toHaveAttribute('aria-selected', 'true');

// Click on the Previews tab
fireEvent.click(getByRole('tab', { name: 'Preview' }));
fireEvent.click(screen.getByRole('tab', { name: 'Preview' }));

// Switch back to the collection
fireEvent.click((await screen.findByText('Collection 1')));

// The Manage (default) tab should be selected because the collection does not have a Preview tab
expect(getByRole('tab', { name: 'Manage' })).toHaveAttribute('aria-selected', 'true');
expect(screen.getByRole('tab', { name: 'Manage' })).toHaveAttribute('aria-selected', 'true');
});

it('can filter by capa problem type', async () => {
Expand Down
59 changes: 34 additions & 25 deletions src/library-authoring/LibraryAuthoringPage.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useState } from 'react';
import { useCallback, useEffect, useState } from 'react';
import { Helmet } from 'react-helmet';
import classNames from 'classnames';
import { StudioFooter } from '@edx/frontend-component-footer';
Expand All @@ -16,12 +16,7 @@ import {
Tabs,
} from '@openedx/paragon';
import { Add, ArrowBack, InfoOutline } from '@openedx/paragon/icons';
import {
Link,
useLocation,
useNavigate,
useSearchParams,
} from 'react-router-dom';
import { Link } from 'react-router-dom';

import Loading from '../generic/Loading';
import SubHeader from '../generic/sub-header/SubHeader';
Expand All @@ -36,11 +31,12 @@ import {
SearchKeywordsField,
SearchSortWidget,
} from '../search-manager';
import LibraryContent, { ContentType } from './LibraryContent';
import LibraryContent from './LibraryContent';
import { LibrarySidebar } from './library-sidebar';
import { useComponentPickerContext } from './common/context/ComponentPickerContext';
import { useLibraryContext } from './common/context/LibraryContext';
import { SidebarBodyComponentId, useSidebarContext } from './common/context/SidebarContext';
import { ContentType, useLibraryRoutes } from './routes';

import messages from './messages';

Expand All @@ -51,7 +47,7 @@ const HeaderActions = () => {

const {
openAddContentSidebar,
openInfoSidebar,
openLibrarySidebar,
closeLibrarySidebar,
sidebarComponentInfo,
} = useSidebarContext();
Expand All @@ -60,13 +56,19 @@ const HeaderActions = () => {

const infoSidebarIsOpen = sidebarComponentInfo?.type === SidebarBodyComponentId.Info;

const handleOnClickInfoSidebar = () => {
const { navigateTo } = useLibraryRoutes();
const handleOnClickInfoSidebar = useCallback(() => {
if (infoSidebarIsOpen) {
closeLibrarySidebar();
} else {
openInfoSidebar();
openLibrarySidebar();
}
};

if (!componentPickerMode) {
// Reset URL to library home
navigateTo();
}
}, [navigateTo, sidebarComponentInfo, closeLibrarySidebar, openLibrarySidebar]);

return (
<div className="header-actions">
Expand Down Expand Up @@ -124,8 +126,6 @@ interface LibraryAuthoringPageProps {

const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPageProps) => {
const intl = useIntl();
const location = useLocation();
const navigate = useNavigate();

const {
isLoadingPage: isLoadingStudioHome,
Expand All @@ -139,29 +139,41 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage
libraryData,
isLoadingLibraryData,
showOnlyPublished,
componentId,
collectionId,
} = useLibraryContext();
const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext();

const { insideCollections, insideComponents, navigateTo } = useLibraryRoutes();

// The activeKey determines the currently selected tab.
const [activeKey, setActiveKey] = useState<ContentType>(ContentType.home);
const getActiveKey = () => {
pomegranited marked this conversation as resolved.
Show resolved Hide resolved
if (insideCollections) {
return ContentType.collections;
}
if (insideComponents) {
return ContentType.components;
}
return ContentType.home;
};

useEffect(() => {
const currentPath = location.pathname.split('/').pop();
const contentType = getActiveKey();

if (componentPickerMode || currentPath === libraryId || currentPath === '') {
if (componentPickerMode) {
setActiveKey(ContentType.home);
} else if (currentPath && currentPath in ContentType) {
setActiveKey(ContentType[currentPath] || ContentType.home);
} else {
setActiveKey(contentType);
}
}, []);

useEffect(() => {
if (!componentPickerMode) {
openInfoSidebar();
openInfoSidebar(componentId, collectionId);
}
}, []);

const [searchParams] = useSearchParams();

if (isLoadingLibraryData) {
return <Loading />;
}
Expand All @@ -181,10 +193,7 @@ const LibraryAuthoringPage = ({ returnToLibrarySelection }: LibraryAuthoringPage
const handleTabChange = (key: ContentType) => {
setActiveKey(key);
if (!componentPickerMode) {
navigate({
pathname: key,
search: searchParams.toString(),
});
navigateTo({ contentType: key });
}
};

Expand Down
7 changes: 1 addition & 6 deletions src/library-authoring/LibraryContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,10 @@ import { useLibraryContext } from './common/context/LibraryContext';
import { useSidebarContext } from './common/context/SidebarContext';
import CollectionCard from './components/CollectionCard';
import ComponentCard from './components/ComponentCard';
import { ContentType } from './routes';
import { useLoadOnScroll } from '../hooks';
import messages from './collections/messages';

export enum ContentType {
home = '',
components = 'components',
collections = 'collections',
}

/**
* Library Content to show content grid
*
Expand Down
60 changes: 38 additions & 22 deletions src/library-authoring/LibraryLayout.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { useCallback } from 'react';
import {
Route,
Routes,
useParams,
useMatch,
useLocation,
} from 'react-router-dom';

import { ROUTES } from './routes';
import LibraryAuthoringPage from './LibraryAuthoringPage';
import { LibraryProvider } from './common/context/LibraryContext';
import { SidebarProvider } from './common/context/SidebarContext';
Expand All @@ -16,43 +18,57 @@ import { ComponentEditorModal } from './components/ComponentEditorModal';
const LibraryLayout = () => {
const { libraryId } = useParams();

const match = useMatch('/library/:libraryId/collection/:collectionId');

const collectionId = match?.params.collectionId;

if (libraryId === undefined) {
// istanbul ignore next - This shouldn't be possible; it's just here to satisfy the type checker.
throw new Error('Error: route is missing libraryId.');
}

return (
const location = useLocation();
const context = useCallback((childPage) => (
<LibraryProvider
/** 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}
/** 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}
libraryId={libraryId}
collectionId={collectionId}
/** The component picker modal to use. We need to pass it as a reference instead of
* directly importing it to avoid the import cycle:
* ComponentPicker > LibraryAuthoringPage/LibraryCollectionPage >
* Sidebar > AddContentContainer > ComponentPicker */
componentPicker={ComponentPicker}
>
<SidebarProvider>
<Routes>
<Route
path="collection/:collectionId"
element={<LibraryCollectionPage />}
/>
<Route
path="*"
element={<LibraryAuthoringPage />}
/>
</Routes>
<CreateCollectionModal />
<ComponentEditorModal />
<>
{childPage}
<CreateCollectionModal />
<ComponentEditorModal />
</>
</SidebarProvider>
</LibraryProvider>
), [location.pathname]);

return (
<Routes>
<Route
path={ROUTES.COMPONENTS}
element={context(<LibraryAuthoringPage />)}
/>
<Route
path={ROUTES.COLLECTIONS}
element={context(<LibraryAuthoringPage />)}
/>
<Route
path={ROUTES.COMPONENT}
element={context(<LibraryAuthoringPage />)}
/>
<Route
path={ROUTES.COLLECTION}
element={context(<LibraryCollectionPage />)}
/>
<Route
path={ROUTES.HOME}
element={context(<LibraryAuthoringPage />)}
/>
</Routes>
Comment on lines +50 to +71
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this entire <Routes>...</Routes> worth having instead of just putting <LibraryAuthoringPage /> ? I guess it will show a 404 error if none of the routes match? Or does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advantage of spelling out all of these routes is that it's now very easy to get the :componentId / :collectionId from useParams() and use these as the defaults in LibraryProvider.

I also had to wrap the page element in that context() wrapper so that the routes would be available when LibraryProvider is created -- previously <LibraryLayout> wrapped <LibraryProvider> around the <Routes>, and so <LibraryProvider> couldn't use them.

);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,13 @@ jest.mock('frontend-components-tinymce-advanced-plugins', () => ({ a11ycheckerCs

const { libraryId } = mockContentLibrary;
const render = (collectionId?: string) => {
const params: { libraryId: string, collectionId?: string } = { libraryId };
if (collectionId) {
params.collectionId = collectionId;
}
const params: { libraryId: string, collectionId?: string } = { libraryId, collectionId };
return baseRender(<AddContentContainer />, {
path: '/library/:libraryId/*',
path: '/library/:libraryId/:collectionId?',
params,
extraWrapper: ({ children }) => (
<LibraryProvider
libraryId={libraryId}
collectionId={collectionId}
>
{ children }
<ComponentEditorModal />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const render = () => baseRender(<PickLibraryContentModal isOpen onClose={onClose
extraWrapper: ({ children }) => (
<LibraryProvider
libraryId={libraryId}
collectionId="collectionId"
componentPicker={ComponentPicker}
>
{children}
Expand Down
Loading
Loading