Skip to content
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

Port the Explorer from Create React App to Next 13 (w/ App Router) #237

Merged
merged 1 commit into from
May 8, 2023

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented May 6, 2023

Apologies in advance for this un-reviewable PR. We're porting Explorer from Create React App to the brand-new-released-yesterday Next 13 with a modern App Router.

In addition to the port, this iteration features:

  • Unique page titles and description metadata
  • Dropdowns that work
  • Repaired NFToken collection pages
  • Newest version of Sentry
  • Tests now run with Jest
  • Update to Node 18
  • Migrate to pnpm
  • Fix an issue where NFT images would transiently fail to load

Lighthouse scores

Without actually fixing many of the underlying problems, the old site on the new infrastructure pulls better Lighthouse scores.

Before After
Homepage image image
USDC Token page image image
Transaction detail page image image

@vercel
Copy link

vercel bot commented May 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 8, 2023 7:16pm

@ngundotra
Copy link
Collaborator

Ship it. Nothing stands out as being immediately wrong

@@ -0,0 +1,4 @@
{
"typescript.tsdk": "node_modules/.pnpm/[email protected]/node_modules/typescript/lib",
"typescript.enablePromptUseWorkspaceTsdk": true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to add this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. Next 13 insists on this somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LOL

@ngundotra
Copy link
Collaborator

Just checking, I see that you've renamed a bunch of files to match their paths, but other than that why are there so many diffs in +/- files (I'd expect mostly moving files)

@steveluscher
Copy link
Collaborator Author

This PR breaks blame again because I didn't take care to git mv every rename. Blame was already discontinued a few months ago, so I don't think there's much benefit to preserving it now.

Copy link
Collaborator

@ngundotra ngundotra left a comment

Choose a reason for hiding this comment

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

LGTM

@steveluscher steveluscher merged commit 6b3f6b7 into master May 8, 2023
@steveluscher steveluscher deleted the next13-port branch May 8, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants