fix(lib-user): use next/link for internal app routes #6379
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eatyourgreens
force-pushed
the
fix-next-links
branch
from
af9eb9c
to
fc363d1
Compare
eatyourgreens
commented
Oct 13, 2024
| @@ -291,7 +292,7 @@ function MainContent({ | |||
| margin={{ top: 'small' }} | |||
| > | |||
| <StyledCertificateButton | |||
| forwardedAs='a' | |||
| forwardedAs={Link} | |||
There was a problem hiding this comment.
Noting here that forwardedAs='a' does nothing on a Grommet Button. Grommet automatically renders Button as a link, if it has a href attribute.
|
I agree with these changes, but just a heads up that full review won't take place until next week. There are changes to AFD cache rules and actions to purge AFD going into place during FEM deploy tomorrow, so I just want to isolate the changes to prefetching in this PR for deploy the next time instead. I included the bump in lib-user's peer dependency for Next.js in #6367. |
Use `next/link` for the home page link to user stats, and the link to your stats certificate. Also fix the `lib-user` peer dependencies, which were still requiring Next.js 13.5.
eatyourgreens
force-pushed
the
fix-next-links
branch
from
fc363d1
to
4ff419a
Compare
|
Sounds good. Here's a quick screen recording to show the instantaneous transitions between page views on this branch, once the initial data load is complete. Screen.Recording.2024-10-17.at.09.10.29.mov |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
next/linkfor the home page link to user stats, the link to your stats certificate, and the Back link at the top of each page.Also fix the
lib-userpeer dependencies, which were still requiring Next.js 13.5.Please request review from
@zooniverse/frontendteam or an individual member of that team.Package
Linked Issue and/or Talk Post
How to Review
'More stats', on the home page, and 'View certificate', on the user stats page, and the Back link on each of those pages, should now use client-side routing in the root Next.js app.
Build the user library with these changes, then build and run the root app with
yarn build && yarn start, to verify that those links are now using client-side routing (ie. the page doesn't reload, andapp-rootdoesn't unmount and remount in the browser, when moving between views.)This should also lower the load on the Next.js app server, since it doesn't have to serve the whole app on every page transition. Locally, you'll probably see that as faster page transitions with a production build.
NB. Next.js development mode disables client-side routing, so I don't think this PR can be checked with
yarn dev.Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrapordocker-compose up --buildand FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Bug Fix