Move index to app router #2224
Move index to app router #2224
Conversation
|
@jonathanawesome is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
jonathanawesome
changed the title
|
Deployment failed with the following error: |
| className="size-16 saturate-[.1] dark:opacity-90 xl:size-32" | ||
| // @ts-expect-error React doesn't know it exists yet, but @types/react do | ||
| fetchpriority="low" | ||
| fetchPriority="low" |
There was a problem hiding this comment.
Did you bump the React version? so... if it's not lowercase and React doesn't know about it, will it still get passed?
There was a problem hiding this comment.
I didn't bump react, but I had the same thought. Switching to camelCase displayed no warning and it I double-checked that it was passing though successfully.
| { children: "FAQ", href: "/conf/2025/#faq" }, | ||
| ]} | ||
| /> | ||
| <ThemeProvider attribute="class"> |
There was a problem hiding this comment.
This means the old conference websites are now inside of ThemeProvider, right?
We should look through them. I don't think they relied on .dark class, but I'll spend a few minutes once the deploy preview is up.
There was a problem hiding this comment.
Ok, i certainly seem to have bungled this bit...I had a feeling there was something about these different theme strategies I was missing. That said, take a look at the live conf/2024...this is broken already.
| <Navbar items={topLevelNavbarItems} /> | ||
| {children} | ||
| <Footer /> |
There was a problem hiding this comment.
Seems you lifted .isolate by accident.
We can try to remove bg-neu-0 text-neu-900 classes if add them to body styles and test the old conference pages, but isolate was there to avoid fighting the z-index (we portal modals outside).
| <Navbar items={topLevelNavbarItems} /> | |
| {children} | |
| <Footer /> | |
| <Navbar items={topLevelNavbarItems} /> | |
| <div className="isolate"> | |
| {children} | |
| </div> | |
| <Footer /> |
hasparus
left a comment
hasparus
left a comment
There was a problem hiding this comment.
looks good, but that isolate got lost (I think there should be a visible bug if you open a navbar submenu on 404 page without it) and it would be nice to have a deploy preview
| import { events, EventCard } from "../../components/events" | ||
| import pinkCircle from "./pink-circle.svg" | ||
| import { Button } from '../../app/conf/_design-system/button' | ||
| import { Button } from '@/app/conf/_design-system/button' |
There was a problem hiding this comment.
I postponed doing this, but in a separate PR we should lift @/app/conf/_design-system to @/_design-system.
It's like this because I did the conf website first, so I worked only in the conf dir.
| --- | ||
|
|
||
| import { Button } from "../../../app/conf/_components/button" | ||
| import { Button } from "@/app/conf/_components/button" |
There was a problem hiding this comment.
WDYT about adding https://github.com/dword-design/eslint-plugin-import-alias...
or actually https://github.com/steelsojka/eslint-import-alias because it's configurable but we'd have to fix the Windows bug.
Ideally I'd like it to match typescript.preferences.importModuleSpecifier: "shortest" setting in VSCode.
| export function IndexPage() { | ||
| return ( | ||
| <div className="gql-all-anchors-focusable bg-neu-0"> | ||
| <Head> |
There was a problem hiding this comment.
oh nice, I see you removed it already, lemme write a suggestion to add it to metadata export then
| https://github.com/vercel/next.js/pull/47328#issuecomment-1488891093 | ||
| */} | ||
| <title> | ||
| Not Found | Please click the button to file a broken-link issue if this |
There was a problem hiding this comment.
Let's make it a bit shorter. It's landing in a browser tab, right?
| Not Found | Please click the button to file a broken-link issue if this | |
| GraphQL | Page Not Found |
| <div className="isolate bg-neu-0 text-neu-900 antialiased"> | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
Let's move it back to where it was as a sibling of footer and navbar.
| <div className="isolate bg-neu-0 text-neu-900 antialiased"> | |
| {children} | |
| </div> | |
| {children} |
| import { IndexPage } from "../../components/index-page" | ||
|
|
||
| export const metadata: Metadata = { | ||
| title: "GraphQL | A query language for your API", |
There was a problem hiding this comment.
oh I just noticed you already removed it from the index page, nice
here it is
| title: "GraphQL | A query language for your API", | |
| title: "GraphQL | A query language for your API", | |
| description: "GraphQL is a query language for APIs and a runtime for fulfilling those queries with your existing data." |
| // conditionally add a key here to avoid react list key errors | ||
| <Tag | ||
| key={!links ? tag : undefined} | ||
| className={opaque ? "bg-neu-0" : ""} | ||
| color={color} | ||
| > |
There was a problem hiding this comment.
oh I can't believe I missed that
mby we could shorten it and add the tag unconditionally
it's the only child so even if it was a long string React won't spend a lot of time comparing it
| // conditionally add a key here to avoid react list key errors | |
| <Tag | |
| key={!links ? tag : undefined} | |
| className={opaque ? "bg-neu-0" : ""} | |
| color={color} | |
| > | |
| <Tag | |
| key={tag} | |
| className={opaque ? "bg-neu-0" : ""} | |
| color={color} | |
| > |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Let's merge #2227 first. Should help with the sidebar issue. |
2 participants
This PR attempts migrates the site index from pages router to app router and sets up the proper layout files.
Notes:
html/head/bodyelements.(main)and(conf)/conf/yearimplement viz-only./approot and can't use our(main)layout.<title>working. We can remove this after we've updated to modern nextjs.