-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[examples] Simplify Next.js example #40661
[examples] Simplify Next.js example #40661
Conversation
This is a [Next.js](https://nextjs.org/) project bootstrapped with [`create-next-app`](https://github.com/vercel/next.js/tree/canary/packages/create-next-app). | ||
This is a [Next.js](https://nextjs.org/) project bootstrapped with [`create-next-app`](https://github.com/vercel/next.js/tree/HEAD/packages/create-next-app). |
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.
Future proof
export default function RootLayout({ children }: { children: React.ReactNode }) { | ||
export default function RootLayout(props: { children: React.ReactNode }) { |
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.
MUI code style convention
return ( | ||
<html lang="en"> | ||
<body> | ||
+ <AppRouterCacheProvider>{children}</AppRouterCacheProvider> | ||
- {props.children} |
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.
missing
/** @type {import('next').NextConfig} */ | ||
const nextConfig = {}; | ||
|
||
module.exports = nextConfig; | ||
export default nextConfig; |
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.
ESM should be the default at this stage, no? It's not what npx create-next-app@latest
outputs but I don't see why it wouldn't make more sense.
<Typography variant="h4" component="h1" gutterBottom> | ||
<Typography variant="h4" component="h1" sx={{ mb: 2 }}> |
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.
We might not want to promote gutterBottom
much. I recall adding this for markdown like formatting, it's arbitrary: #16926. I don't know if it's idiomatic anymore.
<Typography sx={{ mt: 6, mb: 3 }} color="text.secondary"> | ||
<Typography sx={{ mt: 6, mb: 3, color: 'text.secondary' }}> |
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.
Trying to stick to sx prop wherever possible.
images: { | ||
remotePatterns: [ | ||
{ | ||
protocol: 'https', | ||
hostname: 'source.unsplash.com', | ||
port: '', | ||
pathname: '/random', | ||
}, | ||
], | ||
}, |
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.
Don't need this.
}} | ||
> | ||
<Typography variant="h4" component="h1" sx={{ mb: 2 }}> | ||
Material UI - Next.js example in TypeScript |
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.
to match the text on the main page:
Material UI - Next.js example in TypeScript | |
Material UI - Next.js App Router example in TypeScript |
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.
Good call. Eventually I'd really like to see all of these examples get a nicer branded makeover (like Next.js or Vite boilerplates) but until then, this works.
dc10e3c
to
ebc6f4b
Compare
@samuelsycamore This makes sense 👍. I think this PR will help, it makes more examples look identically the same, so easier to update 10 examples at once that are the same than 10 examples that are different. |
While this might not apply to everyone, I hope my feedback below can help other beginners. Before this PR changes, the It showed a glimpse of how a basic app can be structured and organized in a nice way. It was easy to use, build upon and it run smoothly. I think the new example is a bit too simple, especially for people who don't know these technologies. IMHO, it might be better if it kept some of the things from the old |
@gryphon2411 this is a fair point. We are missing steps:
What problem did you face more specifically? |
I saw this example, again, I had to open a PR, I can't imagine why developers would find this helpful: mui/tech-challenge-full-stack#3 (comment).
It's another round of #40318. From #34905, that has been here for a good chunk of time:
There is still a lot we could do to improve the Next.js integration/experience but one step at a time. For example, we will need
getInitColorSchemeScript
to complete the runtime-less CSS-in-JS engine.