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.

Sign up for GitHub

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: prevent LibraryLayout remount #79

Open
wants to merge 2 commits into
base: jill/fal-3984-fix-autofocus
Choose a base branch
from
Open

fix: prevent LibraryLayout remount #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bc0186
fix: prevent LibraryLayout remount
rpenido Jan 16, 2025
645e841
fix: prevent collection navigation on double click
rpenido Jan 16, 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
7 changes: 1 addition & 6 deletions src/library-authoring/LibraryLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {
Route,
Routes,
useParams,
useLocation,
} from 'react-router-dom';

import { ROUTES } from './routes';
Expand All @@ -23,12 +22,8 @@ const LibraryLayout = () => {
throw new Error('Error: route is missing libraryId.');
}

const location = useLocation();
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}
Comment on lines -29 to -31
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes the issue where the LibraryProvider was being remounted when the location.pathname changes.

Copy link
Member

Choose a reason for hiding this comment

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

Why were we trying to force a re-render before?

Copy link
Member Author

Choose a reason for hiding this comment

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

When we navigated to the Collection Page before, the first render still had collectionId = undefined. Remounting the context provider fixed that. It wasn't the best workaround, but it didn't cause many side effects at the time.

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 +39,7 @@ const LibraryLayout = () => {
</>
</SidebarProvider>
</LibraryProvider>
), [location.pathname]);
), []);

return (
<Routes>
Expand Down
4 changes: 3 additions & 1 deletion src/library-authoring/collections/CollectionInfo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
Tabs,
} from '@openedx/paragon';
import { useCallback } from 'react';
import { useNavigate } from 'react-router-dom';

import { useComponentPickerContext } from '../common/context/ComponentPickerContext';
import { useLibraryContext } from '../common/context/LibraryContext';
Expand All @@ -24,6 +25,7 @@

const CollectionInfo = () => {
const intl = useIntl();
const navigate = useNavigate();

const { componentPickerMode } = useComponentPickerContext();
const { libraryId, setCollectionId } = useLibraryContext();
Expand All @@ -48,7 +50,7 @@
if (componentPickerMode) {
setCollectionId(collectionId);
} else {
navigateTo({ collectionId });
navigate(`/library/${libraryId}/collection/${collectionId}`);

Check warning on line 53 in src/library-authoring/collections/CollectionInfo.tsx

View check run for this annotation

Codecov / codecov/patch

src/library-authoring/collections/CollectionInfo.tsx#L53 

Added line #L53 was not covered by tests
Copy link
Member Author

@rpenido rpenido Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I changed this to differentiate "Open the collection on the sidebar" from "Open the collection page".

We could add a parameter to navigateTo to handle this. If we go that route, would be nice also to change here:
/~https://github.com/openedx/frontend-app-authoring/blob/98fbcff842ed760337700e51204e17e940ee7603/src/library-authoring/components/CollectionCard.tsx#L82

I liked the way you did the navigateTo. These hardcoded routes are not nice.

}
}, [componentPickerMode, navigateTo]);

Expand Down
16 changes: 2 additions & 14 deletions src/library-authoring/routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,7 @@ export const useLibraryRoutes = (): LibraryRoutesData => {
route = ROUTES.HOME;
} else if (insideCollections) {
// We're inside the Collections tab,
route = (
(collectionId && collectionId === params.collectionId)
// now open the previously-selected collection,
? ROUTES.COLLECTION
Comment on lines -87 to -88
Copy link
Member Author

Choose a reason for hiding this comment

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

This was causing a redirect to the Collection page on a second click on the collection card.

// or stay there to list all collections, or a selected collection.
: ROUTES.COLLECTIONS
);
route = ROUTES.COLLECTIONS;
} else if (insideCollection) {
// We're viewing a Collection, so stay there,
// and optionally select a component in that collection.
Expand All @@ -103,13 +97,7 @@ export const useLibraryRoutes = (): LibraryRoutesData => {
route = ROUTES.COMPONENT;
} else {
// We're inside the All Content tab,
route = (
(collectionId && collectionId === params.collectionId)
// now open the previously-selected collection
? ROUTES.COLLECTION
// or stay there to list all content, or optionally select a collection.
: ROUTES.HOME
);
route = ROUTES.HOME;
}

const newPath = generatePath(BASE_ROUTE + route, routeParams);
Expand Down
Loading