From cfb4944d43c78dab4d3b896f9c403c42fb9dec57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 2 Jun 2025 16:05:31 -0300 Subject: [PATCH] feat: refactor library routes and add section/subsection tabs (#2039) Adds the "Section" and "Subsections" tabs to the library authoring and refactors our library router hook to fix some ambiguities and solve some bugs. --- .../add-component/AddComponent.jsx | 4 +- src/generic/block-type-utils/index.scss | 9 +- src/index.jsx | 16 +- .../LibraryAuthoringPage.test.tsx | 24 +- .../LibraryAuthoringPage.tsx | 66 ++--- src/library-authoring/LibraryLayout.tsx | 62 +++-- .../__mocks__/library-search.json | 8 +- .../collections/CollectionInfo.tsx | 3 +- .../collections/LibraryCollectionPage.tsx | 9 +- .../common/context/LibraryContext.tsx | 16 +- .../common/context/SidebarContext.tsx | 45 +++- .../component-info/ComponentDetails.test.tsx | 25 +- .../component-info/ComponentPreview.test.tsx | 2 + .../component-picker/ComponentPicker.test.tsx | 86 ++++++- .../components/CollectionCard.test.tsx | 40 +++- .../components/CollectionCard.tsx | 32 ++- .../components/ComponentCard.test.tsx | 2 +- .../components/ComponentCard.tsx | 12 +- .../components/ComponentDeleter.test.tsx | 12 +- .../components/ComponentMenu.tsx | 8 +- .../components/ContainerCard.test.tsx | 45 +++- .../components/ContainerCard.tsx | 44 ++-- src/library-authoring/containers/UnitInfo.tsx | 2 +- .../library-sidebar/LibrarySidebar.tsx | 13 +- src/library-authoring/messages.ts | 10 + src/library-authoring/routes.test.tsx | 226 +++++++++++++++--- src/library-authoring/routes.ts | 216 ++++++++++------- .../units/LibraryUnitBlocks.tsx | 26 +- .../units/LibraryUnitPage.tsx | 29 ++- 29 files changed, 760 insertions(+), 332 deletions(-) diff --git a/src/course-unit/add-component/AddComponent.jsx b/src/course-unit/add-component/AddComponent.jsx index 9e0fd850f..93f052c14 100644 --- a/src/course-unit/add-component/AddComponent.jsx +++ b/src/course-unit/add-component/AddComponent.jsx @@ -14,6 +14,7 @@ import ComponentModalView from './add-component-modals/ComponentModalView'; import AddComponentButton from './add-component-btn'; import messages from './messages'; import { ComponentPicker } from '../../library-authoring/component-picker'; +import { ContentType } from '../../library-authoring/routes'; import { messageTypes } from '../constants'; import { useIframe } from '../../generic/hooks/context/hooks'; import { useEventListener } from '../../generic/hooks'; @@ -228,7 +229,8 @@ const AddComponent = ({ > { } /> } + element={( + + )} /> } + element={( + + )} /> } /> } /> diff --git a/src/library-authoring/LibraryAuthoringPage.test.tsx b/src/library-authoring/LibraryAuthoringPage.test.tsx index 1fc72f368..4a70a52de 100644 --- a/src/library-authoring/LibraryAuthoringPage.test.tsx +++ b/src/library-authoring/LibraryAuthoringPage.test.tsx @@ -216,15 +216,33 @@ describe('', () => { expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); // Navigate to the components tab - fireEvent.click(screen.getByRole('tab', { name: 'Components' })); + const componentsTab = screen.getByRole('tab', { name: 'Components' }); + fireEvent.click(componentsTab); + expect(componentsTab).toHaveAttribute('aria-selected', 'true'); expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); // Navigate to the collections tab - fireEvent.click(screen.getByRole('tab', { name: 'Collections' })); + const collectionsTab = screen.getByRole('tab', { name: 'Collections' }); + fireEvent.click(collectionsTab); + expect(collectionsTab).toHaveAttribute('aria-selected', 'true'); expect(await screen.findByText('No matching collections found in this library.')).toBeInTheDocument(); // Navigate to the units tab - fireEvent.click(screen.getByRole('tab', { name: 'Units' })); + const unitsTab = screen.getByRole('tab', { name: 'Units' }); + fireEvent.click(unitsTab); + expect(unitsTab).toHaveAttribute('aria-selected', 'true'); + expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); + + // Navigate to the subsections tab + const subsectionsTab = screen.getByRole('tab', { name: 'Subsections' }); + fireEvent.click(subsectionsTab); + expect(subsectionsTab).toHaveAttribute('aria-selected', 'true'); + expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); + + // Navigate to the subsections tab + const sectionsTab = screen.getByRole('tab', { name: 'Sections' }); + fireEvent.click(sectionsTab); + expect(sectionsTab).toHaveAttribute('aria-selected', 'true'); expect(await screen.findByText('No matching components found in this library.')).toBeInTheDocument(); // Go back to Home tab diff --git a/src/library-authoring/LibraryAuthoringPage.tsx b/src/library-authoring/LibraryAuthoringPage.tsx index 9d42611c1..5a739643d 100644 --- a/src/library-authoring/LibraryAuthoringPage.tsx +++ b/src/library-authoring/LibraryAuthoringPage.tsx @@ -21,7 +21,7 @@ import { Tabs, } from '@openedx/paragon'; import { Add, ArrowBack, InfoOutline } from '@openedx/paragon/icons'; -import { Link } from 'react-router-dom'; +import { Link, useLocation } from 'react-router-dom'; import Loading from '../generic/Loading'; import SubHeader from '../generic/sub-header/SubHeader'; @@ -72,8 +72,8 @@ const HeaderActions = () => { } if (!componentPickerMode) { - // Reset URL to library home - navigateTo({ componentId: '', collectionId: '', unitId: '' }); + // If not in component picker mode, reset selected item when opening the info sidebar + navigateTo({ selectedItemId: '' }); } }, [navigateTo, sidebarComponentInfo, closeLibrarySidebar, openLibrarySidebar]); @@ -137,6 +137,7 @@ const LibraryAuthoringPage = ({ visibleTabs = allLibraryPageTabs, }: LibraryAuthoringPageProps) => { const intl = useIntl(); + const location = useLocation(); const { isLoadingPage: isLoadingStudioHome, @@ -151,16 +152,15 @@ const LibraryAuthoringPage = ({ isLoadingLibraryData, showOnlyPublished, extraFilter: contextExtraFilter, - componentId, - collectionId, - unitId, } = useLibraryContext(); - const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext(); + const { sidebarComponentInfo } = useSidebarContext(); const { insideCollections, insideComponents, insideUnits, + insideSections, + insideSubsections, navigateTo, } = useLibraryRoutes(); @@ -178,15 +178,31 @@ const LibraryAuthoringPage = ({ if (insideUnits) { return ContentType.units; } + if (insideSubsections) { + return ContentType.subsections; + } + if (insideSections) { + return ContentType.sections; + } return ContentType.home; }; + const [activeKey, setActiveKey] = useState(getActiveKey); useEffect(() => { if (!componentPickerMode) { - openInfoSidebar(componentId, collectionId, unitId); + // Update the active key whenever the route changes. This ensures that the correct tab is selected + // when navigating using the browser's back/forward buttons because it does not trigger a re-render. + setActiveKey(getActiveKey()); } - }, []); + }, [location.key, getActiveKey]); + + const handleTabChange = useCallback((key: ContentType) => { + setActiveKey(key); + if (!componentPickerMode) { + navigateTo({ contentType: key }); + } + }, [navigateTo]); if (isLoadingLibraryData) { return ; @@ -204,13 +220,6 @@ const LibraryAuthoringPage = ({ return ; } - const handleTabChange = (key: ContentType) => { - setActiveKey(key); - if (!componentPickerMode) { - navigateTo({ contentType: key }); - } - }; - const breadcumbs = componentPickerMode && !restrictToLibrary ? ( ( )); @@ -309,7 +315,7 @@ const LibraryAuthoringPage = ({ - {!(insideCollections || insideUnits) && } + {!(onlyOneType) && } { - const { libraryId } = useParams(); +const LibraryLayoutWrapper: React.FC = ({ children }) => { + const { libraryId, collectionId, unitId } = useParams(); 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.'); } - // The top-level route is `${BASE_ROUTE}/*`, so match will always be non-null. - const matchCollection = useMatch(`${BASE_ROUTE}${ROUTES.COLLECTION}`) as PathMatch<'libraryId' | 'collectionId'> | null; - const collectionId = matchCollection?.params.collectionId; - - // The top-level route is `${BASE_ROUTE}/*`, so match will always be non-null. - const matchUnit = useMatch(`${BASE_ROUTE}${ROUTES.UNIT}`) as PathMatch<'libraryId' | 'unitId'> | null; - const unitId = matchUnit?.params.unitId; - - const context = useCallback((childPage) => ( + return ( LibraryAuthoringPage/LibraryCollectionPage > * Sidebar > AddContent > ComponentPicker */ componentPicker={ComponentPicker} > - <> - {childPage} - - - - + {children ?? } + + + - ), [collectionId, unitId]); + ); +}; - return ( - +const LibraryLayout = () => ( + + }> {[ ROUTES.HOME, - ROUTES.COMPONENT, ROUTES.COMPONENTS, ROUTES.COLLECTIONS, ROUTES.UNITS, + ROUTES.SECTIONS, + ROUTES.SUBSECTIONS, ].map((route) => ( )} + Component={LibraryAuthoringPage} /> ))} )} + Component={LibraryCollectionPage} /> )} + Component={LibraryUnitPage} /> - - ); -}; + + +); export default LibraryLayout; diff --git a/src/library-authoring/__mocks__/library-search.json b/src/library-authoring/__mocks__/library-search.json index 9eee97031..45a0d8e4f 100644 --- a/src/library-authoring/__mocks__/library-search.json +++ b/src/library-authoring/__mocks__/library-search.json @@ -517,7 +517,13 @@ "context_key": "lib:Axim:TEST", "org": "Axim", "access_id": "15", - "num_children": "0" + "num_children": "0", + "published": { + "display_name": "Test Unit" + } + }, + "published": { + "display_name": "Test Unit" } } ], diff --git a/src/library-authoring/collections/CollectionInfo.tsx b/src/library-authoring/collections/CollectionInfo.tsx index 45a5352e5..a9d55a912 100644 --- a/src/library-authoring/collections/CollectionInfo.tsx +++ b/src/library-authoring/collections/CollectionInfo.tsx @@ -48,8 +48,7 @@ const CollectionInfo = () => { if (componentPickerMode) { setCollectionId(collectionId); } else { - /* istanbul ignore next */ - navigateTo({ collectionId, doubleClicked: true }); + navigateTo({ collectionId }); } }, [componentPickerMode, navigateTo]); diff --git a/src/library-authoring/collections/LibraryCollectionPage.tsx b/src/library-authoring/collections/LibraryCollectionPage.tsx index 943664788..1d743007d 100644 --- a/src/library-authoring/collections/LibraryCollectionPage.tsx +++ b/src/library-authoring/collections/LibraryCollectionPage.tsx @@ -1,4 +1,3 @@ -import { useEffect } from 'react'; import { StudioFooterSlot } from '@edx/frontend-component-footer'; import { useIntl } from '@edx/frontend-platform/i18n'; import { @@ -110,9 +109,9 @@ const LibraryCollectionPage = () => { const { componentPickerMode } = useComponentPickerContext(); const { - showOnlyPublished, extraFilter: contextExtraFilter, setCollectionId, componentId, + showOnlyPublished, extraFilter: contextExtraFilter, setCollectionId, } = useLibraryContext(); - const { sidebarComponentInfo, openInfoSidebar } = useSidebarContext(); + const { sidebarComponentInfo } = useSidebarContext(); const { data: collectionData, @@ -121,10 +120,6 @@ const LibraryCollectionPage = () => { error, } = useCollection(libraryId, collectionId); - useEffect(() => { - openInfoSidebar(componentId, collectionId, ''); - }, []); - const { data: libraryData, isLoading: isLibLoading } = useContentLibrary(libraryId); // Only show loading if collection data is not fetched from index yet diff --git a/src/library-authoring/common/context/LibraryContext.tsx b/src/library-authoring/common/context/LibraryContext.tsx index 40bd3a69b..8ed408973 100644 --- a/src/library-authoring/common/context/LibraryContext.tsx +++ b/src/library-authoring/common/context/LibraryContext.tsx @@ -29,8 +29,6 @@ export type LibraryContextData = { /** The ID of the current collection/component/unit, on the sidebar OR page */ collectionId: string | undefined; setCollectionId: (collectionId?: string) => void; - componentId: string | undefined; - setComponentId: (componentId?: string) => void; unitId: string | undefined; setUnitId: (unitId?: string) => void; // Only show published components @@ -115,19 +113,13 @@ export const LibraryProvider = ({ const params = useParams(); const { collectionId: urlCollectionId, - componentId: urlComponentId, unitId: urlUnitId, - selectedItemId: urlSelectedItemId, } = params; - const selectedItemIdIsUnit = !!urlSelectedItemId?.startsWith('lct:'); - const [componentId, setComponentId] = useState( - skipUrlUpdate ? undefined : urlComponentId, - ); const [collectionId, setCollectionId] = useState( - skipUrlUpdate ? undefined : urlCollectionId || (!selectedItemIdIsUnit ? urlSelectedItemId : undefined), + skipUrlUpdate ? undefined : urlCollectionId, ); const [unitId, setUnitId] = useState( - skipUrlUpdate ? undefined : urlUnitId || (selectedItemIdIsUnit ? urlSelectedItemId : undefined), + skipUrlUpdate ? undefined : urlUnitId, ); const context = useMemo(() => { @@ -138,8 +130,6 @@ export const LibraryProvider = ({ setCollectionId, unitId, setUnitId, - componentId, - setComponentId, readOnly, isLoadingLibraryData, showOnlyPublished, @@ -163,8 +153,6 @@ export const LibraryProvider = ({ setCollectionId, unitId, setUnitId, - componentId, - setComponentId, readOnly, isLoadingLibraryData, showOnlyPublished, diff --git a/src/library-authoring/common/context/SidebarContext.tsx b/src/library-authoring/common/context/SidebarContext.tsx index 065784287..6317230a4 100644 --- a/src/library-authoring/common/context/SidebarContext.tsx +++ b/src/library-authoring/common/context/SidebarContext.tsx @@ -2,10 +2,15 @@ import { createContext, useCallback, useContext, + useEffect, useMemo, useState, } from 'react'; +import { useParams } from 'react-router-dom'; import { useStateWithUrlSearchParam } from '../../../hooks'; +import { getContainerTypeFromId } from '../../../generic/key-utils'; +import { useComponentPickerContext } from './ComponentPickerContext'; +import { useLibraryContext } from './LibraryContext'; export enum SidebarBodyComponentId { AddContent = 'add-content', @@ -72,7 +77,6 @@ export enum SidebarActions { export type SidebarContextData = { closeLibrarySidebar: () => void; openAddContentSidebar: () => void; - openInfoSidebar: (componentId?: string, collectionId?: string, unitId?: string) => void; openLibrarySidebar: () => void; openCollectionInfoSidebar: (collectionId: string) => void; openComponentInfoSidebar: (usageKey: string) => void; @@ -169,9 +173,37 @@ export const SidebarProvider = ({ }); }, []); - const openInfoSidebar = useCallback((componentId?: string, collectionId?: string, unitId?: string) => { - if (componentId) { - openComponentInfoSidebar(componentId); + // Set the initial sidebar state based on the URL parameters and context. + const { selectedItemId } = useParams(); + const { unitId, collectionId } = useLibraryContext(); + const { componentPickerMode } = useComponentPickerContext(); + + useEffect(() => { + if (initialSidebarComponentInfo) { + // If the sidebar is already open with a selected item, we don't need to do anything. + return; + } + if (componentPickerMode) { + // If we are in component picker mode, we should not open the sidebar automatically. + return; + } + + // Handle selected item id changes + if (selectedItemId) { + const containerType = getContainerTypeFromId(selectedItemId); + if (containerType === 'unit') { + openUnitInfoSidebar(selectedItemId); + } else if (containerType === 'section') { + // istanbul ignore next + // Open section info sidebar + } else if (containerType === 'subsection') { + // istanbul ignore next + // Open subsection info sidebar + } else if (selectedItemId.startsWith('lb:')) { + openComponentInfoSidebar(selectedItemId); + } else { + openCollectionInfoSidebar(selectedItemId); + } } else if (collectionId) { openCollectionInfoSidebar(collectionId); } else if (unitId) { @@ -179,13 +211,12 @@ export const SidebarProvider = ({ } else { openLibrarySidebar(); } - }, []); + }, [selectedItemId]); const context = useMemo(() => { const contextValue = { closeLibrarySidebar, openAddContentSidebar, - openInfoSidebar, openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, @@ -206,7 +237,6 @@ export const SidebarProvider = ({ }, [ closeLibrarySidebar, openAddContentSidebar, - openInfoSidebar, openLibrarySidebar, openComponentInfoSidebar, sidebarComponentInfo, @@ -237,7 +267,6 @@ export function useSidebarContext(): SidebarContextData { return { closeLibrarySidebar: () => {}, openAddContentSidebar: () => {}, - openInfoSidebar: () => {}, openLibrarySidebar: () => {}, openComponentInfoSidebar: () => {}, openCollectionInfoSidebar: () => {}, diff --git a/src/library-authoring/component-info/ComponentDetails.test.tsx b/src/library-authoring/component-info/ComponentDetails.test.tsx index 7f7b1e3bb..ee79aef00 100644 --- a/src/library-authoring/component-info/ComponentDetails.test.tsx +++ b/src/library-authoring/component-info/ComponentDetails.test.tsx @@ -13,6 +13,7 @@ import { mockXBlockAssets, mockXBlockOLX, } from '../data/api.mocks'; +import { LibraryProvider } from '../common/context/LibraryContext'; import { SidebarBodyComponentId, SidebarProvider } from '../common/context/SidebarContext'; import ComponentDetails from './ComponentDetails'; @@ -24,16 +25,24 @@ mockXBlockOLX.applyMock(); mockGetEntityLinks.applyMock(); mockFetchIndexDocuments.applyMock(); +const { + libraryId, +} = mockContentLibrary; + const render = (usageKey: string) => baseRender(, { + path: `/library/${libraryId}/components/${usageKey}`, + params: { libraryId, selectedItemId: usageKey }, extraWrapper: ({ children }) => ( - - {children} - + + + {children} + + ), }); diff --git a/src/library-authoring/component-info/ComponentPreview.test.tsx b/src/library-authoring/component-info/ComponentPreview.test.tsx index 1233ec600..139f9055f 100644 --- a/src/library-authoring/component-info/ComponentPreview.test.tsx +++ b/src/library-authoring/component-info/ComponentPreview.test.tsx @@ -19,6 +19,8 @@ const { const usageKey = mockLibraryBlockMetadata.usageKeyPublished; const render = () => baseRender(, { + path: `/library/${libraryId}/components/${usageKey}`, + params: { libraryId, selectedItemId: usageKey }, extraWrapper: ({ children }) => ( ', () => { mockSearchResult({ ...mockResult }); }); + it('should be able to switch tabs', async () => { + render(); + + expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); + fireEvent.click(screen.getByDisplayValue(/lib:sampletaxonomyorg1:tl1/i)); + + // Wait for the content library to load + await screen.findByText(/Change Library/i); + await waitFor(() => { + expect(screen.getByText('Test Library 1')).toBeInTheDocument(); + expect(screen.queryAllByText('Introduction to Testing')[0]).toBeInTheDocument(); + }); + + // Navigate to the components tab + screen.logTestingPlaygroundURL(); + const componentsTab = screen.getByRole('tab', { name: 'Components' }); + fireEvent.click(componentsTab); + expect(componentsTab).toHaveAttribute('aria-selected', 'true'); + + // Navigate to the collections tab + const collectionsTab = screen.getByRole('tab', { name: 'Collections' }); + fireEvent.click(collectionsTab); + expect(collectionsTab).toHaveAttribute('aria-selected', 'true'); + + // Navigate to the units tab + const unitsTab = screen.getByRole('tab', { name: 'Units' }); + fireEvent.click(unitsTab); + expect(unitsTab).toHaveAttribute('aria-selected', 'true'); + + // Navigate to the subsections tab + const subsectionsTab = screen.getByRole('tab', { name: 'Subsections' }); + fireEvent.click(subsectionsTab); + expect(subsectionsTab).toHaveAttribute('aria-selected', 'true'); + + // Navigate to the subsections tab + const sectionsTab = screen.getByRole('tab', { name: 'Sections' }); + fireEvent.click(sectionsTab); + expect(sectionsTab).toHaveAttribute('aria-selected', 'true'); + + // Go back to Home tab + const allContentTab = screen.getByRole('tab', { name: 'All Content' }); + fireEvent.click(screen.getByRole('tab', { name: 'All Content' })); + expect(allContentTab).toHaveAttribute('aria-selected', 'true'); + }); + it('should pick component using the component card button', async () => { render(); @@ -99,6 +145,44 @@ describe('', () => { }, '*'); }); + it('should open the unit sidebar', async () => { + render(); + + expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); + fireEvent.click(screen.getByDisplayValue(/lib:sampletaxonomyorg1:tl1/i)); + + // Wait for the content library to load + await screen.findByText(/Change Library/i); + expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); + + // Click on the unit card to open the sidebar + fireEvent.click((await screen.findByText('Test Unit'))); + + const sidebar = await screen.findByTestId('library-sidebar'); + expect(sidebar).toBeInTheDocument(); + }); + + it('double clicking a collection should open it', async () => { + render(); + + expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); + fireEvent.click(screen.getByDisplayValue(/lib:sampletaxonomyorg1:tl1/i)); + + // Wait for the content library to load + await screen.findByText(/Change Library/i); + expect(await screen.findByText('Test Library 1')).toBeInTheDocument(); + + // Mock the collection search result + mockSearchResult(mockCollectionResult); + + // Double click on the collection card to open the collection + userEvent.dblClick(screen.queryAllByText('Collection 1')[0]); + + // Wait for the collection to load + await screen.findByText(/Back to Library/i); + await screen.findByText('Introduction to Testing'); + }); + it('should pick component inside a collection using the card', async () => { render(); @@ -117,7 +201,7 @@ describe('', () => { // Mock the collection search result mockSearchResult(mockCollectionResult); - // Click the add component from the component card + // Click the to open the collection fireEvent.click(within(sidebar).getByRole('button', { name: 'Open' })); // Wait for the collection to load diff --git a/src/library-authoring/components/CollectionCard.test.tsx b/src/library-authoring/components/CollectionCard.test.tsx index 2cb43481f..52939b6ab 100644 --- a/src/library-authoring/components/CollectionCard.test.tsx +++ b/src/library-authoring/components/CollectionCard.test.tsx @@ -2,7 +2,7 @@ import userEvent from '@testing-library/user-event'; import type MockAdapter from 'axios-mock-adapter'; import { - initializeMocks, render as baseRender, screen, waitFor, waitForElementToBeRemoved, within, + initializeMocks, render as baseRender, screen, waitFor, waitForElementToBeRemoved, within, fireEvent, } from '../../testUtils'; import { LibraryProvider } from '../common/context/LibraryContext'; import { type CollectionHit } from '../../search-manager'; @@ -10,6 +10,13 @@ import CollectionCard from './CollectionCard'; import messages from './messages'; import { getLibraryCollectionApiUrl, getLibraryCollectionRestoreApiUrl } from '../data/api'; +const mockNavigate = jest.fn(); + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), // use actual for all non-hook parts + useNavigate: () => mockNavigate, +})); + const collectionHitSample: CollectionHit = { id: 'lib-collectionorg1democourse-collection-display-name', type: 'collection', @@ -36,7 +43,11 @@ const collectionHitSample: CollectionHit = { let axiosMock: MockAdapter; let mockShowToast; +const libraryId = 'lib:org1:Demo_Course'; + const render = (ui: React.ReactElement, showOnlyPublished: boolean = false) => baseRender(ui, { + path: '/library/:libraryId', + params: { libraryId }, extraWrapper: ({ children }) => ( ', () => { userEvent.click(screen.getByTestId('collection-card-menu-toggle')); // Open menu item - const openMenuItem = screen.getByRole('link', { name: 'Open' }); + const openMenuItem = screen.getByRole('button', { name: 'Open' }); expect(openMenuItem).toBeInTheDocument(); - expect(openMenuItem).toHaveAttribute('href', '/library/lb:org1:Demo_Course/collection/collection-display-name'); + fireEvent.click(openMenuItem); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith({ + pathname: `/library/${libraryId}/collection/${collectionHitSample.blockId}`, + search: '', + }); + }); + }); + + it('should navigate to the collection if double clicked', async () => { + render(); + + // Card title + const cardTitle = screen.getByText('Collection Display Formated Name'); + expect(cardTitle).toBeInTheDocument(); + userEvent.dblClick(cardTitle); + + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith({ + pathname: `/library/${libraryId}/collection/${collectionHitSample.blockId}`, + search: '', + }); + }); }); it('should show confirmation box, delete collection and show toast to undo deletion', async () => { diff --git a/src/library-authoring/components/CollectionCard.tsx b/src/library-authoring/components/CollectionCard.tsx index 21d5ef5d4..4e6c6535d 100644 --- a/src/library-authoring/components/CollectionCard.tsx +++ b/src/library-authoring/components/CollectionCard.tsx @@ -8,7 +8,6 @@ import { useToggle, } from '@openedx/paragon'; import { MoreVert } from '@openedx/paragon/icons'; -import { Link } from 'react-router-dom'; import { type CollectionHit } from '../../search-manager'; import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; @@ -28,6 +27,7 @@ type CollectionMenuProps = { const CollectionMenu = ({ hit } : CollectionMenuProps) => { const intl = useIntl(); const { showToast } = useContext(ToastContext); + const { navigateTo } = useLibraryRoutes(); const [isDeleteModalOpen, openDeleteModal, closeDeleteModal] = useToggle(false); const { closeLibrarySidebar, sidebarComponentInfo } = useSidebarContext(); const { @@ -70,6 +70,10 @@ const CollectionMenu = ({ hit } : CollectionMenuProps) => { } }, [sidebarComponentInfo?.id]); + const openCollection = useCallback(() => { + navigateTo({ collectionId: blockId }); + }, [blockId, navigateTo]); + return ( <> @@ -83,10 +87,7 @@ const CollectionMenu = ({ hit } : CollectionMenuProps) => { data-testid="collection-card-menu-toggle" /> - + @@ -114,7 +115,7 @@ type CollectionCardProps = { const CollectionCard = ({ hit } : CollectionCardProps) => { const { componentPickerMode } = useComponentPickerContext(); - const { showOnlyPublished, setCollectionId } = useLibraryContext(); + const { setCollectionId, showOnlyPublished } = useLibraryContext(); const { openCollectionInfoSidebar, sidebarComponentInfo } = useSidebarContext(); const { @@ -136,17 +137,24 @@ const CollectionCard = ({ hit } : CollectionCardProps) => { && sidebarComponentInfo.id === collectionId; const { navigateTo } = useLibraryRoutes(); - const openCollection = useCallback((e?: React.MouseEvent) => { - openCollectionInfoSidebar(collectionId); + const selectCollection = useCallback((e?: React.MouseEvent) => { const doubleClicked = (e?.detail || 0) > 1; if (!componentPickerMode) { - navigateTo({ collectionId, doubleClicked }); + if (doubleClicked) { + navigateTo({ collectionId }); + } else { + navigateTo({ selectedItemId: collectionId }); + } + + // In component picker mode, we want to open the sidebar or the collection + // without changing the URL } else if (doubleClicked) { - /* istanbul ignore next */ setCollectionId(collectionId); + } else { + openCollectionInfoSidebar(collectionId); } - }, [collectionId, navigateTo, openCollectionInfoSidebar]); + }, [collectionId, navigateTo, openCollectionInfoSidebar, setCollectionId, componentPickerMode]); return ( { )} - onSelect={openCollection} + onSelect={selectCollection} selected={selected} /> ); diff --git a/src/library-authoring/components/ComponentCard.test.tsx b/src/library-authoring/components/ComponentCard.test.tsx index dd56c30f8..a3475b525 100644 --- a/src/library-authoring/components/ComponentCard.test.tsx +++ b/src/library-authoring/components/ComponentCard.test.tsx @@ -130,7 +130,7 @@ describe('', () => { fireEvent.click(editOption); // Verify that the url is updated to component url i.e. component is selected expect(mockNavigate).toHaveBeenCalledWith({ - pathname: `/library/${libraryId}/component/${contentHit.usageKey}`, + pathname: `/library/${libraryId}/${contentHit.usageKey}`, search: '', }); }); diff --git a/src/library-authoring/components/ComponentCard.tsx b/src/library-authoring/components/ComponentCard.tsx index 4b58147fb..65c449b5f 100644 --- a/src/library-authoring/components/ComponentCard.tsx +++ b/src/library-authoring/components/ComponentCard.tsx @@ -36,11 +36,13 @@ const ComponentCard = ({ hit }: ComponentCardProps) => { ) ?? ''; const { navigateTo } = useLibraryRoutes(); - const openComponent = useCallback(() => { - openComponentInfoSidebar(usageKey); - + const selectComponent = useCallback(() => { if (!componentPickerMode) { - navigateTo({ componentId: usageKey }); + navigateTo({ selectedItemId: usageKey }); + } else { + // In component picker mode, we want to open the sidebar + // without changing the URL + openComponentInfoSidebar(usageKey); } }, [usageKey, navigateTo, openComponentInfoSidebar]); @@ -63,7 +65,7 @@ const ComponentCard = ({ hit }: ComponentCardProps) => { )} hasUnpublishedChanges={publishStatus !== PublishStatus.Published} - onSelect={openComponent} + onSelect={selectComponent} selected={selected} /> ); diff --git a/src/library-authoring/components/ComponentDeleter.test.tsx b/src/library-authoring/components/ComponentDeleter.test.tsx index f6af3d4a7..4bc1137db 100644 --- a/src/library-authoring/components/ComponentDeleter.test.tsx +++ b/src/library-authoring/components/ComponentDeleter.test.tsx @@ -6,6 +6,7 @@ import { initializeMocks, waitFor, } from '../../testUtils'; +import { LibraryProvider } from '../common/context/LibraryContext'; import { SidebarProvider } from '../common/context/SidebarContext'; import { mockContentLibrary, mockDeleteLibraryBlock, mockLibraryBlockMetadata, mockRestoreLibraryBlock, @@ -19,10 +20,19 @@ mockContentSearchConfig.applyMock(); const mockDelete = mockDeleteLibraryBlock.applyMock(); const mockRestore = mockRestoreLibraryBlock.applyMock(); +const { libraryId } = mockContentLibrary; const usageKey = mockLibraryBlockMetadata.usageKeyPublished; const renderArgs = { - extraWrapper: SidebarProvider, + path: '/library/:libraryId', + params: { libraryId }, + extraWrapper: ({ children }) => ( + + + { children } + + + ), }; let mockShowToast: { (message: string, action?: ToastActionData | undefined): void; mock?: any; }; diff --git a/src/library-authoring/components/ComponentMenu.tsx b/src/library-authoring/components/ComponentMenu.tsx index b5bd16749..7e80007c0 100644 --- a/src/library-authoring/components/ComponentMenu.tsx +++ b/src/library-authoring/components/ComponentMenu.tsx @@ -91,10 +91,9 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { }; const handleEdit = useCallback(() => { - navigateTo({ componentId: usageKey }); - openComponentInfoSidebar(usageKey); + navigateTo({ selectedItemId: usageKey }); openComponentEditor(usageKey); - }, [usageKey]); + }, [usageKey, navigateTo]); const scheduleJumpToCollection = useRunOnNextRender(() => { // TODO: Ugly hack to make sure sidebar shows add to collection section @@ -103,8 +102,7 @@ export const ComponentMenu = ({ usageKey }: { usageKey: string }) => { }); const showManageCollections = useCallback(() => { - navigateTo({ componentId: usageKey }); - openComponentInfoSidebar(usageKey); + navigateTo({ selectedItemId: usageKey }); scheduleJumpToCollection(); }, [ scheduleJumpToCollection, diff --git a/src/library-authoring/components/ContainerCard.test.tsx b/src/library-authoring/components/ContainerCard.test.tsx index a8716d7ad..a33df12db 100644 --- a/src/library-authoring/components/ContainerCard.test.tsx +++ b/src/library-authoring/components/ContainerCard.test.tsx @@ -11,6 +11,13 @@ import { type ContainerHit, PublishStatus } from '../../search-manager'; import ContainerCard from './ContainerCard'; import { getLibraryContainerApiUrl, getLibraryContainerRestoreApiUrl } from '../data/api'; +const mockNavigate = jest.fn(); + +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), // use actual for all non-hook parts + useNavigate: () => mockNavigate, +})); + const containerHitSample: ContainerHit = { id: 'lctorg1democourse-unit-display-name-123', type: 'library_container', @@ -36,15 +43,20 @@ const containerHitSample: ContainerHit = { tags: {}, publishStatus: PublishStatus.Published, }; + +const libraryId = 'lib:Axim:TEST'; + let axiosMock: MockAdapter; let mockShowToast; mockContentLibrary.applyMock(); const render = (ui: React.ReactElement, showOnlyPublished: boolean = false) => baseRender(ui, { + path: '/library/:libraryId', + params: { libraryId }, extraWrapper: ({ children }) => ( {children} @@ -60,14 +72,14 @@ describe('', () => { it('should render the card with title', () => { render(); - expect(screen.queryByText('Unit Display Formated Name')).toBeInTheDocument(); + expect(screen.getByText('Unit Display Formated Name')).toBeInTheDocument(); expect(screen.queryByText('2')).toBeInTheDocument(); // Component count }); it('should render published content', () => { render(, true); - expect(screen.queryByText('Published Unit Display Name')).toBeInTheDocument(); + expect(screen.getByText('Published Unit Display Name')).toBeInTheDocument(); expect(screen.queryByText('1')).toBeInTheDocument(); // Published Component Count }); @@ -79,14 +91,29 @@ describe('', () => { userEvent.click(screen.getByTestId('container-card-menu-toggle')); // Open menu item - const openMenuItem = screen.getByRole('link', { name: 'Open' }); + const openMenuItem = screen.getByRole('button', { name: 'Open' }); expect(openMenuItem).toBeInTheDocument(); - // TODO: To be implemented - // expect(openMenuItem).toHaveAttribute( - // 'href', - // '/library/lb:org1:Demo_Course/container/container-display-name-123', - // ); + fireEvent.click(openMenuItem); + + expect(mockNavigate).toHaveBeenCalledWith({ + pathname: `/library/${libraryId}/unit/${containerHitSample.usageKey}`, + search: '', + }); + }); + + it('should navigate to the container if double clicked', async () => { + render(); + + // Card title + const cardTitle = screen.getByText('Unit Display Formated Name'); + expect(cardTitle).toBeInTheDocument(); + userEvent.dblClick(cardTitle); + + expect(mockNavigate).toHaveBeenCalledWith({ + pathname: `/library/${libraryId}/unit/${containerHitSample.usageKey}`, + search: '', + }); }); it('should delete the container from the menu & restore the container', async () => { diff --git a/src/library-authoring/components/ContainerCard.tsx b/src/library-authoring/components/ContainerCard.tsx index ebcbbbeaf..6b79d4730 100644 --- a/src/library-authoring/components/ContainerCard.tsx +++ b/src/library-authoring/components/ContainerCard.tsx @@ -9,7 +9,6 @@ import { Stack, } from '@openedx/paragon'; import { MoreVert } from '@openedx/paragon/icons'; -import { Link } from 'react-router-dom'; import { getItemIcon, getComponentStyleColor } from '../../generic/block-type-utils'; import { getBlockType } from '../../generic/key-utils'; @@ -33,7 +32,6 @@ type ContainerMenuProps = { const ContainerMenu = ({ hit } : ContainerMenuProps) => { const intl = useIntl(); const { - contextKey, usageKey: containerId, displayName, } = hit; @@ -69,11 +67,14 @@ const ContainerMenu = ({ hit } : ContainerMenuProps) => { }); const showManageCollections = useCallback(() => { - navigateTo({ unitId: containerId }); - openUnitInfoSidebar(containerId); + navigateTo({ selectedItemId: containerId }); scheduleJumpToCollection(); }, [scheduleJumpToCollection, navigateTo, openUnitInfoSidebar, containerId]); + const openContainer = useCallback(() => { + navigateTo({ unitId: containerId }); + }, [navigateTo, containerId]); + return ( <> @@ -87,10 +88,7 @@ const ContainerMenu = ({ hit } : ContainerMenuProps) => { data-testid="container-card-menu-toggle" /> - + @@ -173,7 +171,7 @@ type ContainerCardProps = { const ContainerCard = ({ hit } : ContainerCardProps) => { const { componentPickerMode } = useComponentPickerContext(); - const { setUnitId, showOnlyPublished } = useLibraryContext(); + const { showOnlyPublished } = useLibraryContext(); const { openUnitInfoSidebar, sidebarComponentInfo } = useSidebarContext(); const { @@ -183,7 +181,7 @@ const ContainerCard = ({ hit } : ContainerCardProps) => { numChildren, published, publishStatus, - usageKey: unitId, + usageKey: containerId, content, } = hit; @@ -200,19 +198,25 @@ const ContainerCard = ({ hit } : ContainerCardProps) => { ) ?? []; const selected = sidebarComponentInfo?.type === SidebarBodyComponentId.UnitInfo - && sidebarComponentInfo.id === unitId; + && sidebarComponentInfo.id === containerId; const { navigateTo } = useLibraryRoutes(); - const openContainer = useCallback((e?: React.MouseEvent) => { - if (itemType === 'unit') { - openUnitInfoSidebar(unitId); - setUnitId(unitId); - if (!componentPickerMode) { - navigateTo({ unitId, doubleClicked: (e?.detail || 0) > 1 }); + const selectContainer = useCallback((e?: React.MouseEvent) => { + const doubleClicked = (e?.detail || 0) > 1; + + if (!componentPickerMode) { + if (doubleClicked) { + navigateTo({ unitId: containerId }); + } else { + navigateTo({ selectedItemId: containerId }); } + } else { + // In component picker mode, we want to open the sidebar + // without changing the URL + openUnitInfoSidebar(containerId); } - }, [unitId, itemType, openUnitInfoSidebar, navigateTo]); + }, [containerId, itemType, openUnitInfoSidebar, navigateTo]); return ( { actions={( {componentPickerMode ? ( - + ) : ( )} )} hasUnpublishedChanges={publishStatus !== PublishStatus.Published} - onSelect={openContainer} + onSelect={selectContainer} selected={selected} /> ); diff --git a/src/library-authoring/containers/UnitInfo.tsx b/src/library-authoring/containers/UnitInfo.tsx index cdf5c2d08..3437fba1b 100644 --- a/src/library-authoring/containers/UnitInfo.tsx +++ b/src/library-authoring/containers/UnitInfo.tsx @@ -164,7 +164,7 @@ const UnitInfo = () => { > {renderTab( UNIT_INFO_TABS.Preview, - , + , intl.formatMessage(messages.previewTabTitle), )} {renderTab(UNIT_INFO_TABS.Manage, , intl.formatMessage(messages.manageTabTitle))} diff --git a/src/library-authoring/library-sidebar/LibrarySidebar.tsx b/src/library-authoring/library-sidebar/LibrarySidebar.tsx index aeae34097..ddcddfafd 100644 --- a/src/library-authoring/library-sidebar/LibrarySidebar.tsx +++ b/src/library-authoring/library-sidebar/LibrarySidebar.tsx @@ -17,10 +17,6 @@ import { ComponentInfo, ComponentInfoHeader } from '../component-info'; import { LibraryInfo, LibraryInfoHeader } from '../library-info'; import messages from '../messages'; -interface LibrarySidebarProps { - onSidebarClose?: () => void; -} - /** * Sidebar container for library pages. * @@ -30,7 +26,7 @@ interface LibrarySidebarProps { * You can add more components in `bodyComponentMap`. * Use the returned actions to open and close this sidebar. */ -const LibrarySidebar = ({ onSidebarClose }: LibrarySidebarProps) => { +const LibrarySidebar = () => { const intl = useIntl(); const { sidebarAction, @@ -71,11 +67,6 @@ const LibrarySidebar = ({ onSidebarClose }: LibrarySidebarProps) => { const buildBody = () : React.ReactNode => bodyComponentMap[sidebarComponentInfo?.type || 'unknown']; const buildHeader = (): React.ReactNode => headerComponentMap[sidebarComponentInfo?.type || 'unknown']; - const handleSidebarClose = () => { - closeLibrarySidebar(); - onSidebarClose?.(); - }; - return ( @@ -85,7 +76,7 @@ const LibrarySidebar = ({ onSidebarClose }: LibrarySidebarProps) => { src={Close} iconAs={Icon} alt={intl.formatMessage(messages.closeButtonAlt)} - onClick={handleSidebarClose} + onClick={closeLibrarySidebar} size="inline" /> diff --git a/src/library-authoring/messages.ts b/src/library-authoring/messages.ts index 428838645..66e336c1d 100644 --- a/src/library-authoring/messages.ts +++ b/src/library-authoring/messages.ts @@ -51,6 +51,16 @@ const messages = defineMessages({ defaultMessage: 'Units', description: 'Tab label for the units tab', }, + subsectionsTab: { + id: 'course-authoring.library-authoring.subsections-tab', + defaultMessage: 'Subsections', + description: 'Tab label for the subsections tab', + }, + sectionsTab: { + id: 'course-authoring.library-authoring.sections-tab', + defaultMessage: 'Sections', + description: 'Tab label for the sections tab', + }, componentsTempPlaceholder: { id: 'course-authoring.library-authoring.components-temp-placeholder', defaultMessage: 'There are {componentCount} components in this library', diff --git a/src/library-authoring/routes.test.tsx b/src/library-authoring/routes.test.tsx index ad03cbc4c..57aff79cf 100644 --- a/src/library-authoring/routes.test.tsx +++ b/src/library-authoring/routes.test.tsx @@ -85,22 +85,24 @@ describe('Library Authoring routes', () => { }, destination: { params: { - componentId: 'cmptId', + selectedItemId: 'lb:org:lib:cmpt', }, - path: '/component/cmptId', + path: '/lb:org:lib:cmpt', }, }, { label: 'from All Content tab > selected Component, select a different Component', origin: { - path: '', - params: {}, + path: '/lb:org:lib:cmpt1', + params: { + selectedItemId: 'lb:org:lib:cmpt1', + }, }, destination: { params: { - componentId: 'cmptId2', + selectedItemId: 'lb:org:lib:cmpt2', }, - path: '/component/cmptId2', + path: '/lb:org:lib:cmpt2', }, }, { @@ -111,7 +113,7 @@ describe('Library Authoring routes', () => { }, destination: { params: { - collectionId: 'clctnId', + selectedItemId: 'clctnId', }, path: '/clctnId', }, @@ -124,11 +126,26 @@ describe('Library Authoring routes', () => { }, destination: { params: { - unitId: 'lct:org:lib:unit:unitId', + selectedItemId: 'lct:org:lib:unit:unitId', }, path: '/lct:org:lib:unit:unitId', }, }, + { + label: 'from All Content tab > selected unit, navigate to unit page', + origin: { + path: '/lct:org:lib:unit:unitId', + params: { + selectedItemId: 'lct:org:lib:unit:unitId', + }, + }, + destination: { + params: { + unitId: 'lct:org:lib:unit:unitId', + }, + path: '/unit/lct:org:lib:unit:unitId', + }, + }, { label: 'navigate from All Content > selected Collection to the Collection page', origin: { @@ -141,24 +158,20 @@ describe('Library Authoring routes', () => { params: { collectionId: 'clctnId', }, - /* - * Note: the MemoryRouter used by testUtils breaks this, but should be: - * path: '/collection/clctnId', - */ - path: '/clctnId', + path: '/collection/clctnId', }, }, { label: 'from All Content > Collection, select a different Collection', origin: { params: { - collectionId: 'clctnId', + selectedItemId: 'clctnId', }, path: '/clctnId', }, destination: { params: { - collectionId: 'clctnId2', + selectedItemId: 'clctnId2', }, path: '/clctnId2', }, @@ -177,15 +190,29 @@ describe('Library Authoring routes', () => { }, }, }, + { + label: 'from All Content tab > select a Component, navigate to Component page', + origin: { + path: '/lb:org:lib:cmpt', + params: { + selectedItemId: 'lb:org:lib:cmpt', + }, + }, + destination: { + path: '/components/lb:org:lib:cmpt', // Should keep the selected component + params: { + contentType: ContentType.components, + selectedItemId: 'lb:org:lib:cmpt', + }, + }, + }, { label: 'navigate from Components tab to Collections tab', origin: { - label: 'Components tab', path: '/components', params: {}, }, destination: { - label: 'Collections tab', params: { contentType: ContentType.collections, }, @@ -200,9 +227,9 @@ describe('Library Authoring routes', () => { }, destination: { params: { - componentId: 'cmptId', + selectedItemId: 'lb:org:lib:cmpt', }, - path: '/components/cmptId', + path: '/components/lb:org:lib:cmpt', }, }, // "Collections" tab @@ -219,6 +246,22 @@ describe('Library Authoring routes', () => { }, }, }, + { + label: 'navigate From All Content tab with component selected, to Collection tab', + origin: { + params: { + selectedItemId: 'lb:org:lib:component', + }, + path: '/lb:org:lib:component', + }, + destination: { + params: { + contentType: ContentType.collections, + selectedItemId: 'lb:org:lib:component', + }, + path: '/collections', // Should ignore the selected component + }, + }, { label: 'navigate from Collections tab to Components tab', origin: { @@ -240,7 +283,7 @@ describe('Library Authoring routes', () => { }, destination: { params: { - collectionId: 'clctnId', + selectedItemId: 'clctnId', }, path: '/collections/clctnId', }, @@ -251,17 +294,13 @@ describe('Library Authoring routes', () => { params: { selectedItemId: 'clctnId', }, - path: '/collections/clctnId', + path: '/clctnId', }, destination: { params: { collectionId: 'clctnId', }, - /* - * Note: the MemoryRouter used by testUtils breaks this, but should be: - * path: '/collection/clctnId', - */ - path: '/collections/clctnId', + path: '/collection/clctnId', }, }, { @@ -270,13 +309,13 @@ describe('Library Authoring routes', () => { params: { collectionId: 'clctnId', }, - path: '/collections/clctnId', + path: '/collection/clctnId', }, destination: { params: { collectionId: 'clctnId2', }, - path: '/collections/clctnId2', + path: '/collection/clctnId2', }, }, // "Units" tab @@ -301,7 +340,7 @@ describe('Library Authoring routes', () => { }, destination: { params: { - unitId: 'unitId', + selectedItemId: 'unitId', }, path: '/units/unitId', }, @@ -319,6 +358,134 @@ describe('Library Authoring routes', () => { }, }, }, + { + label: 'navigate From All Content tab with component selected, to Units tab', + origin: { + path: '/lb:org:lib:component', + params: { + selectedItemId: 'lb:org:lib:component', + }, + }, + destination: { + params: { + contentType: ContentType.units, + selectedItemId: 'lb:org:lib:component', + }, + path: '/units', // Should ignore the selected component + }, + }, + // "Sections" tab + { + label: 'navigate from All Content tab to Sections tab', + origin: { + path: '', + params: {}, + }, + destination: { + path: '/sections', + params: { + contentType: ContentType.sections, + }, + }, + }, + { + label: 'from Sections tab, select a Section', + origin: { + path: '/sections', + params: {}, + }, + destination: { + params: { + selectedItemId: 'sectionId', + }, + path: '/sections/sectionId', + }, + }, + { + label: 'navigate from Sections tab to All Content tab', + origin: { + path: '/sections', + params: {}, + }, + destination: { + path: '', + params: { + contentType: ContentType.home, + }, + }, + }, + { + label: 'navigate From All Content tab with component selected, to Sections tab', + origin: { + path: '/lb:org:lib:component', + params: { + selectedItemId: 'lb:org:lib:component', + }, + }, + destination: { + params: { + contentType: ContentType.sections, + selectedItemId: 'lb:org:lib:component', + }, + path: '/sections', // Should ignore the selected component + }, + }, + // "Subsections" tab + { + label: 'navigate from All Content tab to Subsections tab', + origin: { + path: '', + params: {}, + }, + destination: { + path: '/subsections', + params: { + contentType: ContentType.subsections, + }, + }, + }, + { + label: 'from Sections tab, select a Subsection', + origin: { + path: '/subsections', + params: {}, + }, + destination: { + params: { + selectedItemId: 'subsectionId', + }, + path: '/subsections/subsectionId', + }, + }, + { + label: 'navigate from Subsections tab to All Content tab', + origin: { + path: '/subsections', + params: {}, + }, + destination: { + path: '', + params: { + contentType: ContentType.home, + }, + }, + }, + { + label: 'navigate From All Content tab with component selected, to Subsections tab', + origin: { + path: '/lb:org:lib:component', + params: { + selectedItemId: 'lb:org:lib:component', + }, + }, + destination: { + path: '/subsections', // Should ignore the selected component + params: { + contentType: ContentType.subsections, + selectedItemId: 'lb:org:lib:component', + }, + }, + }, ])( '$label', async ({ origin, destination }) => { @@ -336,6 +503,7 @@ describe('Library Authoring routes', () => { path: `/library/:libraryId${origin.path}/*`, params: { libraryId: mockContentLibrary.libraryId, + unitId: '', collectionId: '', selectedItemId: '', ...origin.params, diff --git a/src/library-authoring/routes.ts b/src/library-authoring/routes.ts index 541648372..a5409e5cf 100644 --- a/src/library-authoring/routes.ts +++ b/src/library-authoring/routes.ts @@ -1,7 +1,7 @@ /** * Constants and utility hook for the Library Authoring routes. */ -import { useCallback } from 'react'; +import { useCallback, useMemo } from 'react'; import { generatePath, matchPath, @@ -11,29 +11,26 @@ import { useSearchParams, type PathMatch, } from 'react-router-dom'; -import { useLibraryContext } from './common/context/LibraryContext'; export const BASE_ROUTE = '/library/:libraryId'; export const ROUTES = { // LibraryAuthoringPage routes: - // * Components tab, with an optionally selected componentId in the sidebar. - COMPONENTS: '/components/:componentId?', + // * Components tab, with an optionally selected component in the sidebar. + COMPONENTS: '/components/:selectedItemId?', // * Collections tab, with an optionally selected collectionId in the sidebar. - COLLECTIONS: '/collections/:collectionId?', + COLLECTIONS: '/collections/:selectedItemId?', // * Sections tab, with an optionally selected sectionId in the sidebar. - SECTIONS: '/sections/:sectionId?', + SECTIONS: '/sections/:selectedItemId?', // * Subsections tab, with an optionally selected subsectionId in the sidebar. - SUBSECTIONS: '/subsections/:subsectionId?', + SUBSECTIONS: '/subsections/:selectedItemId?', // * Units tab, with an optionally selected unitId in the sidebar. - UNITS: '/units/:unitId?', - // * All Content tab, with an optionally selected componentId in the sidebar. - COMPONENT: '/component/:componentId', + UNITS: '/units/:selectedItemId?', // * All Content tab, with an optionally selected collection or unit in the sidebar. HOME: '/:selectedItemId?', // LibraryCollectionPage route: // * with a selected collectionId and/or an optionally selected componentId. - COLLECTION: '/collection/:collectionId/:componentId?', + COLLECTION: '/collection/:collectionId/:selectedItemId?', // LibrarySectionPage route: // * with a selected sectionId and/or an optionally selected subsectionId. SECTION: '/section/:sectionId/:subsectionId?', @@ -42,7 +39,7 @@ export const ROUTES = { SUBSECTION: '/subsection/:subsectionId/:unitId?', // LibraryUnitPage route: // * with a selected unitId and/or an optionally selected componentId. - UNIT: '/unit/:unitId/:componentId?', + UNIT: '/unit/:unitId/:selectedItemId?', }; export enum ContentType { @@ -50,16 +47,17 @@ export enum ContentType { collections = 'collections', components = 'components', units = 'units', + subsections = 'subsections', + sections = 'sections', } export const allLibraryPageTabs: ContentType[] = Object.values(ContentType); export type NavigateToData = { - componentId?: string, + selectedItemId?: string, collectionId?: string, contentType?: ContentType, unitId?: string, - doubleClicked?: boolean, }; export type LibraryRoutesData = { @@ -73,7 +71,10 @@ export type LibraryRoutesData = { insideUnits: PathMatch | null; insideUnit: PathMatch | null; - // Navigate using the best route from the current location for the given parameters. + /** Navigate using the best route from the current location for the given parameters. + * This function can be mutated if there are changes in the current route, so always include + * it in the dependencies array if used on a `useCallback`. + */ navigateTo: (dict?: NavigateToData) => void; }; @@ -82,7 +83,6 @@ export const useLibraryRoutes = (): LibraryRoutesData => { const params = useParams(); const [searchParams] = useSearchParams(); const navigate = useNavigate(); - const { setComponentId, setUnitId, setCollectionId } = useLibraryContext(); const insideCollection = matchPath(BASE_ROUTE + ROUTES.COLLECTION, pathname); const insideCollections = matchPath(BASE_ROUTE + ROUTES.COLLECTIONS, pathname); @@ -94,115 +94,142 @@ export const useLibraryRoutes = (): LibraryRoutesData => { const insideUnits = matchPath(BASE_ROUTE + ROUTES.UNITS, pathname); const insideUnit = matchPath(BASE_ROUTE + ROUTES.UNIT, pathname); + // Sanity check to ensure that we are not inside more than one route at the same time. + // istanbul ignore if: this is a developer error, not a user error. + if ( + [ + insideCollection, + insideCollections, + insideComponents, + insideSections, + insideSection, + insideSubsections, + insideSubsection, + insideUnits, + insideUnit, + ].filter((match): match is PathMatch => match !== null).length > 1) { + throw new Error('Cannot be inside more than one route at the same time.'); + } + + /** This function is used to navigate to a specific route based on the provided parameters. + */ const navigateTo = useCallback(({ - componentId, + selectedItemId, collectionId, unitId, contentType, - doubleClicked, }: NavigateToData = {}) => { - const { - collectionId: urlCollectionId, - componentId: urlComponentId, - unitId: urlUnitId, - selectedItemId: urlSelectedItemId, - } = params; - const routeParams = { ...params, - // Overwrite the current componentId/collectionId params if provided - ...((componentId !== undefined) && { componentId }), - ...((collectionId !== undefined) && { collectionId, selectedItemId: collectionId }), - ...((unitId !== undefined) && { unitId, selectedItemId: unitId }), - ...(contentType === ContentType.home && { selectedItemId: urlCollectionId || urlUnitId }), - ...(contentType === ContentType.components && { componentId: urlComponentId || urlSelectedItemId }), - ...(contentType === ContentType.collections && { collectionId: urlCollectionId || urlSelectedItemId }), - ...(contentType === ContentType.units && { unitId: urlUnitId || urlSelectedItemId }), + // Overwrite the params with the provided values. + ...((selectedItemId !== undefined) && { selectedItemId }), + ...((unitId !== undefined) && { unitId }), + ...((collectionId !== undefined) && { collectionId }), }; let route: string; - // Update componentId, unitId, collectionId in library context if is not undefined. - // Ids can be cleared from route by passing in empty string so we need to set it. - if (componentId !== undefined) { - setComponentId(componentId); - } - if (unitId !== undefined) { - setUnitId(unitId); - } - if (collectionId !== undefined) { - setCollectionId(collectionId); + if (routeParams.selectedItemId + && (['components', 'units', 'sections', 'subsections'].includes(routeParams.selectedItemId || ''))) { + // These are not valid selectedItemIds, but routes + routeParams.selectedItemId = undefined; } + // Update unitId/collectionId in library context if is not undefined. + // Ids can be cleared from route by passing in empty string so we need to set it. + if (unitId !== undefined) { + routeParams.selectedItemId = undefined; + + // If we can have a unitId alongside a routeParams.collectionId, it means we are inside a collection + // trying to navigate to a unit, so we want to clear the collectionId to not have ambiquity. + if (routeParams.collectionId !== undefined) { + routeParams.collectionId = undefined; + } + } else if (collectionId !== undefined) { + routeParams.selectedItemId = undefined; + } else if (contentType) { + // We are navigating to the library home, so we need to clear the unitId and collectionId + routeParams.unitId = undefined; + routeParams.collectionId = undefined; + } + + // The code below determines the best route to navigate to based on the + // current pathname and the provided parameters. // Providing contentType overrides the current route so we can change tabs. if (contentType === ContentType.components) { + if (!routeParams.selectedItemId?.startsWith('lb:')) { + // If the selectedItemId is not a component, we need to set it to undefined + routeParams.selectedItemId = undefined; + } route = ROUTES.COMPONENTS; } else if (contentType === ContentType.collections) { + // FIXME: We are using the Collection key, not the full OpaqueKey. So we + // can't directly use the selectedItemId to determine if it's a collection. + // We need to change this to use the full OpaqueKey in the future. + if (routeParams.selectedItemId?.includes(':unit:') + || routeParams.selectedItemId?.includes(':subsection:') + || routeParams.selectedItemId?.includes(':section:') + || routeParams.selectedItemId?.startsWith('lb:')) { + routeParams.selectedItemId = undefined; + } route = ROUTES.COLLECTIONS; } else if (contentType === ContentType.units) { + if (!routeParams.selectedItemId?.includes(':unit:')) { + // Clear selectedItemId if it is not a unit. + routeParams.selectedItemId = undefined; + } route = ROUTES.UNITS; + } else if (contentType === ContentType.subsections) { + if (!routeParams.selectedItemId?.includes(':subsection:')) { + // If the selectedItemId is not a subsection, we need to set it to undefined + routeParams.selectedItemId = undefined; + } + route = ROUTES.SUBSECTIONS; + } else if (contentType === ContentType.sections) { + if (!routeParams.selectedItemId?.includes(':section:')) { + // If the selectedItemId is not a section, we need to set it to undefined + routeParams.selectedItemId = undefined; + } + route = ROUTES.SECTIONS; } else if (contentType === ContentType.home) { route = ROUTES.HOME; - } else if (insideCollections) { - // We're inside the Collections tab, - route = ( - (collectionId && doubleClicked) - // now open the previously-selected collection, - ? ROUTES.COLLECTION - // or stay there to list all collections, or a selected collection. - : ROUTES.COLLECTIONS - ); - } else if (insideCollection) { - // We're viewing a Collection, so stay there, - // and optionally select a component in that collection. + } else if (routeParams.unitId) { + route = ROUTES.UNIT; + } else if (routeParams.collectionId) { route = ROUTES.COLLECTION; + // From here, we will just stay in the current route } else if (insideComponents) { - // We're inside the Components tab, so stay there, - // optionally selecting a component. route = ROUTES.COMPONENTS; + } else if (insideCollections) { + route = ROUTES.COLLECTIONS; } else if (insideUnits) { - // We're inside the units tab, - route = ( - (unitId && doubleClicked) - // now open the previously-selected unit, - ? ROUTES.UNIT - // or stay there to list all units, or a selected unit. - : ROUTES.UNITS - ); - } else if (insideUnit) { - // We're viewing a Unit, so stay there, - // and optionally select a component in that Unit. - route = ROUTES.UNIT; - } else if (componentId) { - // We're inside the All Content tab, so stay there, - // and select a component. - route = ROUTES.COMPONENT; - } else if (collectionId && doubleClicked) { - // now open the previously-selected collection - route = ROUTES.COLLECTION; - } else if (unitId && doubleClicked) { - // now open the previously-selected unit - route = ROUTES.UNIT; + route = ROUTES.UNITS; + } else if (insideSubsections) { + route = ROUTES.SUBSECTIONS; + } else if (insideSections) { + route = ROUTES.SECTIONS; } else { - // or stay there to list all content, or optionally select a collection. route = ROUTES.HOME; } + // Also remove the `sa` (sidebar action) search param if it exists. + searchParams.delete('sa'); + const newPath = generatePath(BASE_ROUTE + route, routeParams); - navigate({ - pathname: newPath, - search: searchParams.toString(), - }); + // Prevent unnecessary navigation if the path is the same. + if (newPath !== pathname) { + navigate({ + pathname: newPath, + search: searchParams.toString(), + }); + } }, [ navigate, params, searchParams, pathname, - setComponentId, - setUnitId, - setCollectionId, ]); - return { + return useMemo(() => ({ navigateTo, insideCollection, insideCollections, @@ -213,5 +240,16 @@ export const useLibraryRoutes = (): LibraryRoutesData => { insideSubsection, insideUnits, insideUnit, - }; + }), [ + navigateTo, + insideCollection, + insideCollections, + insideComponents, + insideSections, + insideSection, + insideSubsections, + insideSubsection, + insideUnits, + insideUnit, + ]); }; diff --git a/src/library-authoring/units/LibraryUnitBlocks.tsx b/src/library-authoring/units/LibraryUnitBlocks.tsx index e2d654cac..8d1791a64 100644 --- a/src/library-authoring/units/LibraryUnitBlocks.tsx +++ b/src/library-authoring/units/LibraryUnitBlocks.tsx @@ -59,7 +59,7 @@ const BlockHeader = ({ block, readOnly }: ComponentBlockProps) => { const { showOnlyPublished } = useLibraryContext(); const { showToast } = useContext(ToastContext); const { navigateTo } = useLibraryRoutes(); - const { openComponentInfoSidebar, setSidebarAction } = useSidebarContext(); + const { setSidebarAction } = useSidebarContext(); const updateMutation = useUpdateXBlockFields(block.originalId); @@ -86,8 +86,7 @@ const BlockHeader = ({ block, readOnly }: ComponentBlockProps) => { /* istanbul ignore next */ const jumpToManageTags = () => { - navigateTo({ componentId: block.originalId }); - openComponentInfoSidebar(block.originalId); + navigateTo({ selectedItemId: block.originalId }); scheduleJumpToTags(); }; @@ -137,23 +136,17 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => const { showOnlyPublished } = useLibraryContext(); const { navigateTo } = useLibraryRoutes(); - const { - unitId, collectionId, componentId, openComponentEditor, - } = useLibraryContext(); - - const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext(); + const { openComponentEditor } = useLibraryContext(); + const { sidebarComponentInfo } = useSidebarContext(); const handleComponentSelection = useCallback((numberOfClicks: number) => { - navigateTo({ componentId: block.originalId }); + navigateTo({ selectedItemId: block.originalId }); const canEdit = canEditComponent(block.originalId); if (numberOfClicks > 1 && canEdit) { // Open editor on double click. openComponentEditor(block.originalId); - } else { - // open current component sidebar - openInfoSidebar(block.originalId, collectionId, unitId); } - }, [block, collectionId, unitId, navigateTo, canEditComponent, openComponentEditor, openInfoSidebar]); + }, [block, navigateTo, canEditComponent, openComponentEditor]); useEffect(() => { if (block.isNew) { @@ -178,7 +171,7 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => }; } return {}; - }, [isDragging, componentId, block]); + }, [isDragging, block]); const selected = sidebarComponentInfo?.type === SidebarBodyComponentId.ComponentInfo && sidebarComponentInfo?.id === block.originalId; @@ -221,13 +214,14 @@ const ComponentBlock = ({ block, readOnly, isDragging }: ComponentBlockProps) => }; interface LibraryUnitBlocksProps { + unitId: string; /** set to true if it is rendered as preview * This disables drag and drop, title edit and menus */ readOnly?: boolean; } -export const LibraryUnitBlocks = ({ readOnly: componentReadOnly }: LibraryUnitBlocksProps) => { +export const LibraryUnitBlocks = ({ unitId, readOnly: componentReadOnly }: LibraryUnitBlocksProps) => { const intl = useIntl(); const [orderedBlocks, setOrderedBlocks] = useState([]); const [isAddLibraryContentModalOpen, showAddLibraryContentModal, closeAddLibraryContentModal] = useToggle(); @@ -235,7 +229,7 @@ export const LibraryUnitBlocks = ({ readOnly: componentReadOnly }: LibraryUnitBl const [hidePreviewFor, setHidePreviewFor] = useState(null); const { showToast } = useContext(ToastContext); - const { unitId, readOnly: libraryReadOnly, showOnlyPublished } = useLibraryContext(); + const { readOnly: libraryReadOnly, showOnlyPublished } = useLibraryContext(); const readOnly = componentReadOnly || libraryReadOnly; diff --git a/src/library-authoring/units/LibraryUnitPage.tsx b/src/library-authoring/units/LibraryUnitPage.tsx index c8d337314..bdcb449ea 100644 --- a/src/library-authoring/units/LibraryUnitPage.tsx +++ b/src/library-authoring/units/LibraryUnitPage.tsx @@ -16,6 +16,7 @@ import ErrorAlert from '../../generic/alert-error'; import { InplaceTextEditor } from '../../generic/inplace-text-editor'; import { ToastContext } from '../../generic/toast-context'; import Header from '../../header'; +import { useComponentPickerContext } from '../common/context/ComponentPickerContext'; import { useLibraryContext } from '../common/context/LibraryContext'; import { COLLECTION_INFO_TABS, COMPONENT_INFO_TABS, SidebarBodyComponentId, UNIT_INFO_TABS, useSidebarContext, @@ -70,6 +71,7 @@ const EditableTitle = ({ unitId }: EditableTitleProps) => { const HeaderActions = () => { const intl = useIntl(); + const { componentPickerMode } = useComponentPickerContext(); const { unitId, readOnly } = useLibraryContext(); const { openAddContentSidebar, @@ -93,7 +95,10 @@ const HeaderActions = () => { } else { openUnitInfoSidebar(unitId); } - navigateTo({ unitId, componentId: '' }); + + if (!componentPickerMode) { + navigateTo({ unitId }); + } }, [unitId, infoSidebarIsOpen]); return ( @@ -125,24 +130,18 @@ export const LibraryUnitPage = () => { const { libraryId, unitId, - componentId, - collectionId, } = useLibraryContext(); + + // istanbul ignore if: this should never happen + if (!unitId) { + throw new Error('unitId is required'); + } + const { - openInfoSidebar, sidebarComponentInfo, setDefaultTab, setHiddenTabs, } = useSidebarContext(); - const { navigateTo } = useLibraryRoutes(); - - // Open unit or component sidebar on mount - useEffect(() => { - // includes componentId to open correct sidebar on page mount from url - openInfoSidebar(componentId, collectionId, unitId); - // avoid including componentId in dependencies to prevent flicker on closing sidebar. - // See below useEffect that clears componentId on closing sidebar. - }, [unitId, collectionId]); useEffect(() => { setDefaultTab({ @@ -230,7 +229,7 @@ export const LibraryUnitPage = () => { /> - + @@ -239,7 +238,7 @@ export const LibraryUnitPage = () => { className="library-authoring-sidebar box-shadow-left-1 bg-white" data-testid="library-sidebar" > - navigateTo({ componentId: '' })} /> + )}