Next.js Migration Bug Fixes #1626
Conversation
… shouldForwardProp)
…able on router.asPath
ChristopherChudzicki
left a comment
ChristopherChudzicki
left a comment
There was a problem hiding this comment.
One small TS request and a question about the error change.
Otherwise, this looks good to me.
BTW: An alternative to the useRouter stuff may have been to use window.history.pushState.
Unlike React Router, direct usage of window.history.pushState seems OK / sanctioned / encouraged in NextJS for things that are truly client-only.
In a very minor sense, opening/closing the drawer is NOT client-only, since apparently it's the server that updates the page metadata title via an RSC request
| // Note that { scroll: true } and { scroll: false } both remove the hash fragment | ||
| if (hash) { | ||
| router.push(`?${newParams}${hash}`) | ||
| } else { | ||
| // Prevent scroll to top of page | ||
| router.push(`?${newParams}${hash}`, { scroll: false }) | ||
| } |
There was a problem hiding this comment.
Is the if block here so that upon closing the drawer, you scroll to the doc fragment? IMO that isn't desirable. But as discussed, this is a very edgy case.
There was a problem hiding this comment.
The thinking here is if there is a hash, we're specifying a point on the incoming page to scroll to, so we should not disable scroll. The actual issue was that we want to preserve the hash, but need to fix an issue with the page scrolling on closing the drawer. Providing any options to router.push() { scroll: false|true } removes the hash.
I have removed the ${hash} bit though as it doesn't make sense in the else block (always "").
There was a problem hiding this comment.
The thinking here is if there is a hash, we're specifying a point on the incoming page to scroll to, so we should not disable scroll
That doesn't seem right to me....Close drawer should be "Restore state before drawer was opened (including scrol)" not "Restore state when page first loaded".
Approving. this seems very much like an edge case to me.
| return newSearchParams | ||
| } | ||
|
|
||
| // TODO Not used anywhere in project. Remove? |
There was a problem hiding this comment.
You could remove it. This dates from when we opened the drawer via a click handler, as opposed to the whole card being a link.
| type LinkProps = ButtonStyleProps & React.ComponentProps<typeof Link> | ||
|
|
||
| const buildStyles = ( | ||
| props: (ButtonProps | AnchorProps | LinkProps) & { theme: Theme }, |
There was a problem hiding this comment.
Any objection to leaving the types as-is, namely
const buildStyles = (props: ButtonStyleProps & { theme: Theme }) => {?
buildStyles shouldn't use, e.g., className or onClick, which both exist on ButtonProps | AnchorProps | LinkProps... buildStyles should only use the style-related props.
There was a problem hiding this comment.
No - I was beefing with Typescript on this branch before the merge from main. They are indeed not needed.
| return <NotFoundPage /> | ||
| } | ||
| if (this.state.hasError && isForbiddenError(this.state.error)) { | ||
| return <ForbiddenPage /> |
There was a problem hiding this comment.
What is this change for? If I visit http://localhost:8062/c/unit/cow now I get 500 error.
Which... apparently I also get at https://next.rc.learn.mit.edu/c/unit/cow, though the UI shows the 404 page
There was a problem hiding this comment.
Opened https://github.com/mitodl/hq/issues/5734 for the current 500 situation
There was a problem hiding this comment.
The issue was that it was trapping all errors and displaying the 404 page, not the error.
We have a not-found.tsx in the app root to display the 404 page for routes not matched.
The request to a /c/unit/noexist (a matched route for a non existing channel) results in an error thrown while making the API request server side to fetch the channel details. We get a 404 from the API, though as it is not handled it produces a 500 at the browser with the error shown.
Here we need to handle and replay the 404. Errors thrown in server components or root layouts are not caught by the error boundary - this is likely fixed by adding an app/error.ts (server components) and app/global-error.ts, the widest catch-all (info).
Our current wraps children within the inner page - we should widen this for the whole page or remove if app/error.ts has the same effect.
Copied above to https://github.com/mitodl/hq/issues/5734
There was a problem hiding this comment.
Let's handle #5734 in a dedicated error-handling PR? Though maybe leave this as-was for now
There was a problem hiding this comment.
Yes - it's deserving of a separate issue. We need to:
-
Reinstate 404 handling in the client component error boundary, but not for all errors (Check instance of AxiosError and status 404). This is to display a 404 if we open a not found resource drawer. I was finding this resulted in infinite retry (Is React Query retrying after errors?)
-
Add a catch all error handler for server components (
app/error.ts) to render the 404 page or display the error.
What are the relevant tickets?
Fixes tasklist bugs in https://github.com/mitodl/hq/issues/5395
Description (What does it do?)
Fixes:
Logo not clickable in header
Opening a resource drawer scrolls to the top of the page
Test: Opening and closing the drawer should not scroll the background page (e.g. scroll down the homepage or search results and click a card).
Additional context:
next/linkLinks (to open drawers) accept ascrollboolean, as doesnext/navigation'srouter.push()(to close drawers) to disable the default scroll to top, though this also removes hash fragments (browsers do not send these to the server and they are intended for scroll navigation to anchors within a page). Setting scroll to false will also cause the page not to scroll to the top when navigating between routes, which is not what we want. On Links this PR checks for an href stating with?(changing search params, not navigating between routes) to disable scroll. Related, it updates our ol-test-utilities/mocks/nextNavigation.ts for theuseSearchParamshook to match thenext/navigationbehavior - smoothing the delta between Node.js'ParsedUrlQueryandURLSearchParams.External link icon in drawer CTA is missing (fixed start and end icons in button links)
Test: Icon should be visible.
Additional context: Issue here was with using @emotion's
withComponent()alongside theshouldForwardPropfilter - props were not passed to children (endIconin this case).Alignment of search icon in navigation
Test: Header search icon position should match designs
Additional context: This is semi-redundant after sync with main as the header design has been updated.
URL hash part is not preserved on drawer open/close
Fixes Department/Topic detail pages get re-directed to rc.learn.mit.edu
Test: Click on any chip link in /topics or list item in /departments. You should remain on the next.rc.learn.mit.edu domain.
Additional context: We reuse the RC API, which returns fully qualified URLs for channel items. As these always link to the same domain, this PR extracts the path to use as link hrefs.