-
Notifications
You must be signed in to change notification settings - Fork 25
fix: page routes wildcard which breaks tutor #322
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,7 +45,7 @@ subscribe(APP_READY, () => { | |||||
ReactDOM.render( | ||||||
<AppProvider store={store}> | ||||||
<Routes> | ||||||
<Route path={`${ROUTES.Detail.HOME}/*`} element={<StudioHeaderWrapper />} /> | ||||||
<Route path={ROUTES.Detail.HOME} element={<StudioHeaderWrapper />} /> | ||||||
<Route path="*" element={<StudioHeaderWrapper />} /> | ||||||
</Routes> | ||||||
<Routes> | ||||||
|
@@ -61,7 +61,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 />} /> | ||||||
<Route path={`${ROUTES.Block.HOME}`} element={<LibraryBlockPage />} /> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't revert this one, this route has child routes There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 😦
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Questions: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?