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

[docs] Update nextjs-typescript-example #29974

Merged
merged 10 commits into from
Dec 2, 2021

Conversation

huydhoang
Copy link
Contributor

Hi,
I'm starting a new project with MUI v5 and found this Next.js with Typescript example to be a bit outdated.
It featured non-standard Next.js folder structure compared to one generated with create-next-app.
So I've updated it and tested locally before pushing to a new branch.
Hopefully this would be useful to new MUI users!


Update: This is a cleaned up version of #29814
Please consider merging it @mnajdova

@mui-pr-bot
Copy link

mui-pr-bot commented Nov 30, 2021

No bundle size changes

Generated by 🚫 dangerJS against 2aae7cc

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good overall, added one comment. Next time, no need to create new PRs, seeing the history and the conversation helps when reviewing

examples/nextjs-with-typescript/src/pages/_app.tsx Outdated Show resolved Hide resolved
@huydhoang
Copy link
Contributor Author

Looks good overall, added one comment. Next time, no need to create new PRs, seeing the history and the conversation helps when reviewing

Well noted :)

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I pushed a commit to fix an issue with wrong import, and moved the ./styles directory under ./src no need for it to be outside of it. Looks good now 👍 Thanks!

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2021
@mnajdova mnajdova merged commit 4ef7c20 into mui:master Dec 2, 2021
@mnajdova mnajdova added docs Improvements or additions to the documentation examples Relating to /examples labels Dec 2, 2021
@@ -4,8 +4,8 @@ import { AppProps } from 'next/app';
import { ThemeProvider } from '@mui/material/styles';
Copy link
Member

Choose a reason for hiding this comment

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

The change of folder location is wrong, /pages is supposed to be at the same level as the package.json https://nextjs.org/docs/basic-features/pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oliviertassinari I mean, Next.js might not be that opiniated but if /pages is at the same level as package.json, then there shouldn't be a /src. What's now in /src should instead be moved into /components and /lib.

Please take a look at this community post on Dev:
https://dev.to/vadorequest/a-2021-guide-about-structuring-your-next-js-project-in-a-flexible-and-efficient-way-472

@@ -15,6 +15,7 @@ export default class MyDocument extends Document {
rel="stylesheet"
href="https://fonts.googleapis.com/css?family=Roboto:300,400,500,700&display=swap"
/>
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons" />
Copy link
Member

Choose a reason for hiding this comment

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

We don't encourage font icons. I'm removing it in #30381.

Suggested change
<link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons" />

@@ -19,7 +19,7 @@ export default function MyApp(props: MyAppProps) {
return (
<CacheProvider value={emotionCache}>
<Head>
<title>My page</title>
<title>Change title in _app.tsx</title>
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this should be inside each page. I'm removing it in #30381.

noLinkStyle?: boolean;
} & Omit<NextLinkComposedProps, 'to' | 'linkAs' | 'href'> &
Omit<MuiLinkProps, 'href'>;

// A styled version of the Next.js Link component:
// https://nextjs.org/docs/api-reference/next/link
// https://nextjs.org/docs/#with-link
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, the previous link was correct. I'm fixing it in #30381.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation examples Relating to /examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants