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

[material-ui-nextjs] Add Link integration component #40265

Closed
wants to merge 9 commits into from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Dec 21, 2023

Preview: https://deploy-preview-40265--material-ui.netlify.app/material-ui/guides/nextjs/#link

Summary

Added Link for App Router and Pages Router (Both routers have different import for useRouter, so the split is required).

Usage:

// src/app/*
import { Link } from '@mui/material-nextjs/v14-appRouter';


// src/pages/*
import { Link } from '@mui/material-nextjs/v14-pagesRouter';

Related to #30858

@siriwatknp siriwatknp added new feature New feature or request package: material-ui Specific to @mui/material nextjs labels Dec 21, 2023
@mui-bot
Copy link

mui-bot commented Dec 21, 2023

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2c4af10

@siriwatknp siriwatknp marked this pull request as ready for review December 22, 2023 09:00
}}
// below props are required for NextLink to work with MUI Link
passHref
legacyBehavior
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should see if we can avoid this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so based on Next.js docs.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 8, 2024
@siriwatknp siriwatknp requested a review from mnajdova January 8, 2024 02:59
@siriwatknp
Copy link
Member Author

@mnajdova @mj12albert in case you missed the notification, please review it.

Comment on lines +12 to +14
/**
* Extra class name to apply to the link.
*/
Copy link
Member

Choose a reason for hiding this comment

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

What we usually use for the className:

Suggested change
/**
* Extra class name to apply to the link.
*/

Did this mui/mui-x#11693 accordingly.

}}
// below props are required for NextLink to work with MUI Link
passHref
legacyBehavior
Copy link
Member

@oliviertassinari oliviertassinari Jan 16, 2024

Choose a reason for hiding this comment

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

I think we should update to the latest stable API, part of is:

Suggested change
legacyBehavior

I don't see why it's needed, I would expect <MdLink component={NextLink}> to work fine.

Copy link
Member

Choose a reason for hiding this comment

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

For pages router, use the Link only from `@mui/material-nextjs/*-pagesRouter`:

```js
import { Link } from '@mui/material-nextjs/v13-pagesRouter'; // or `v14-pagesRouter` if you are using Next.js v14
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also document how to use a Link with a Button.

import Button from '@mui/material/Button';
import NextLink from 'next/link';

<Button component={NextLink} href="/foo">

Actually, I don't know if this works (maybe types issue, maybe href prop conflict) but this is definitely an important use case.

Copy link

Choose a reason for hiding this comment

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

Also it should be possible to pass in NextLink via a theme, right? Should we have that example as well?

Comment on lines +87 to +90
:::warning
Do not use the Link from <b>`@mui/material-nextjs/*-pagesRouter`</b> for App Router.
:::

Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Suggested change
:::warning
Do not use the Link from <b>`@mui/material-nextjs/*-pagesRouter`</b> for App Router.
:::

Because we import the router, it will crash if it uses the wrong API

Screenshot 2024-01-16 at 01 21 57

You can pass any props supported by both components to the Link.

```js
import { Link } from '@mui/material-nextjs/v13-appRouter'; // or `v14-appRouter` if you are using Next.js v14
Copy link
Member

Choose a reason for hiding this comment

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

This creates a horizontal scrollbar, and I think better if set to be generic.

Suggested change
import { Link } from '@mui/material-nextjs/v13-appRouter'; // or `v14-appRouter` if you are using Next.js v14
import { Link } from '@mui/material-nextjs/v13-appRouter';
// or `v1X-appRouter` if you are using Next.js v1X

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

The Link implementation we use on our docs got a bit simpler #40940, the gap with this one is reduced.

@siriwatknp
Copy link
Member Author

I'll move this to v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request nextjs package: material-ui Specific to @mui/material PR: out-of-date The pull request has merge conflicts and can't be merged v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants