-
Notifications
You must be signed in to change notification settings - Fork 5
CORE-1221: Fix layoutParameters issue #2773
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
CORE-1221: Fix layoutParameters issue #2773
Conversation
f7d62b3 to
42d2f77
Compare
42d2f77 to
48a5d9a
Compare
|
|
||
| if (['books', 'textbooks'].includes(dir)) { | ||
| return ( | ||
| return path === '' ? <Error404 /> : ( |
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.
Earlier changes to the router had /books coming up as a blank page when it should 404.
Things like /books/Calculus will reroute to /details/books/Calculus.
| ); | ||
| const Layout = React.useCallback( | ||
| ({children}: React.PropsWithChildren<object>) => { | ||
| const layoutParameterName = layoutParameters.name; |
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.
This eliminates the need for the cast. In theory, layoutParameters.name could change before the function call.
| if (layoutParameterName === null) { | ||
| return <div>{children}</div>; | ||
| } | ||
| const LoadableLayout = loadable({ |
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.
Defining this separately was causing the bug that is the main thing being addressed by this PR.
|
|
||
| export function LayoutUsingData({data, children}: {data: FlexPageData, children: React.ReactNode}) { | ||
| function warnAndUseDefault() { | ||
| console.warn('No layout set for 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.
This makes the issue more obvious. When no layout is defined for the page in the CMS, the page wouldn't render. Now it will render with the default layout, and there will also be a warning.
| import FeaturedResourcesSection from '~/pages/details/common/featured-resources/featured-resources'; | ||
| import ShellContextProvider from '../../../../helpers/shell-context'; | ||
| import {MemoryRouter} from 'react-router-dom'; | ||
| import MemoryRouter from '~/../../test/helpers/future-memory-router'; |
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.
This is just to quiet a bunch of warnings in testing.
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.
Seems reasonable
CORE-1221