Skip to content
This repository has been archived by the owner on Jul 18, 2024. It is now read-only.

fix: page routes wildcard which breaks tutor #322

Closed
wants to merge 3 commits into from
Closed

fix: page routes wildcard which breaks tutor #322

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
44195fb
fix: page routes wildcard which breaks tutor
connorhaugh Sep 26, 2023
e36f1da
style: redundant interpolation
kdmccormick Feb 2, 2024
0952f2b
Merge branch 'master' into fix--tutor-page-routes-broken-after-react-…
kdmccormick Feb 2, 2024
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
4 changes: 2 additions & 2 deletions src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ subscribe(APP_READY, () => {
ReactDOM.render(
<AppProvider store={store}>
<Routes>
<Route path={`${ROUTES.Detail.HOME}/*`} element={<StudioHeaderWrapper />} />
<Route path={`${ROUTES.Detail.HOME}`} element={<StudioHeaderWrapper />} />
kdmccormick marked this conversation as resolved.
Show resolved Hide resolved
<Route path="*" element={<StudioHeaderWrapper />} />
</Routes>
<Routes>
Expand All @@ -52,7 +52,7 @@ subscribe(APP_READY, () => {
<Route path={ROUTES.Detail.EDIT} element={<LibraryEditPage />} />
<Route path={ROUTES.Detail.ACCESS} element={<LibraryAccessPage />} />
<Route path={ROUTES.Detail.IMPORT} element={<CourseImportPage />} />
<Route path={`${ROUTES.Block.HOME}/*`} element={<LibraryBlockPage />} />
Copy link

@ormsbee ormsbee Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@kdmccormick: I lost track of this when openedx/frontend-platform#574 seemed to make things work on tutor. But I'm wondering if this could be the cause of the component edit page still being broken on Tutor dev?

<Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />
Copy link
Contributor

Choose a reason for hiding this comment

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

don't revert this one, this route has child routes

Copy link
Contributor

Choose a reason for hiding this comment

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

@ormsbee any chance you could test this out on Tutor with the wildcard in this route?

Copy link

Choose a reason for hiding this comment

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

Sure.

Copy link

Choose a reason for hiding this comment

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

I actually get the same error either way. Not sure if I'm doing something wrong. 😦

react.development.js:220 Warning: Failed prop type: The prop `library` is marked as required in `StudioHeaderWrapperBase`, but its value is `null`.
    at StudioHeaderWrapperBase (webpack-internal:///./src/library-authoring/studio-header-wrapper/StudioHeaderWrapper.jsx:53:7)
    at ShimmedIntlComponent (webpack-internal:///./node_modules/@edx/frontend-platform/i18n/injectIntlWithShim.js:151:7)
    at injectIntl(ShimmedIntlComponent)
    at ConnectFunction (webpack-internal:///./node_modules/react-redux/es/components/connectAdvanced.js:233:68)
    at RenderedRoute (webpack-internal:///./node_modules/react-router/dist/index.js:554:5)
    at Routes (webpack-internal:///./node_modules/react-router/dist/index.js:1185:5)
    at Router (webpack-internal:///./node_modules/react-router/dist/index.js:1123:15)
    at BrowserRouter (webpack-internal:///./node_modules/react-router-dom/dist/index.js:399:5)
    at Provider (webpack-internal:///./node_modules/react-redux/es/components/Provider.js:18:20)
    at OptionalReduxProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/OptionalReduxProvider.js:18:20)
    at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:139:5)
    at IntlProvider (webpack-internal:///./node_modules/react-intl/lib/src/components/provider.js:92:47)
    at AppProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/AppProvider.js:108:20)

Copy link

@ormsbee ormsbee Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, so that is a warning, and if I hardcode the path for LibraryListPage to be "library-authoring", it does render–even though that warning still spills out in the console.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. It just seems very odd that react-router would overwrite that instead of appending.

Based on the change in how links are handled I thought it might be in https://github.com/openedx/frontend-app-library-authoring/blob/master/src/library-authoring/utils/hoc.jsx but nothing there stood out to me.

Maybe @arbrandes has some insight?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't, but perhaps @Syed-Ali-Abbas-Zaidi has sufficient context. The router-v6 changes were pretty extensive!

Copy link
Contributor

Choose a reason for hiding this comment

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

That they were! To add context, I was specifically looking into why the link Dave mentioned was behaving differently after changing from

  goToCreateLibraryPage = () => {
    this.props.history.push(ROUTES.List.CREATE);
  };

to

  goToCreateLibraryPage = () => {
    this.props.navigate(ROUTES.List.CREATE);
  };

which is what got me looking into hoc.jsx

Copy link
Contributor Author

@connorhaugh connorhaugh Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Questions:
Did you restart/rebuild after making the changes?
Just to confirm, we never tested the problem as fixed on your local tutor?

Copy link

Choose a reason for hiding this comment

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

I did restart/rebuild between changes. Going back to the commit before the react-router upgrade (so commit 16414ff) works. Grabbing the latest master fails. Grabbing this branch also fails in the same way master is failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} />
<Route path={ROUTES.Block.HOME} element={<LibraryBlockPage />} />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair. Going to wait for Dave's test then I'll put up an amended version.

<Route path="*" element={<NotFoundPage />} />
</Route>
</Routes>
Expand Down