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

NextJS: Support next/navigation in Next.js v13 #20065

Merged
merged 22 commits into from
Dec 7, 2022

Conversation

valentinpalkovic
Copy link
Contributor

@valentinpalkovic valentinpalkovic commented Dec 2, 2022

What I did

Support next/navigation in Next.js v13.

How to test

  • Is this testable with Jest or Chromatic screenshots?

If your answer is yes to any of these, please make sure to include it in your PR.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This looks awesome!

@valentinpalkovic valentinpalkovic force-pushed the valentin/document-next-link-component branch from 5fa671a to 544d8e5 Compare December 6, 2022 08:28
@valentinpalkovic valentinpalkovic changed the base branch from valentin/document-next-link-component to next December 6, 2022 08:29
@yannbf yannbf added the nextjs label Dec 6, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Great work!!! Small changes & questions below

code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
code/frameworks/nextjs/README.md Outdated Show resolved Hide resolved
});

test.describe('next/navigation', () => {
test.skip(
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what this will do to the CI status report? cc @kasperpeulen

Copy link
Member

Choose a reason for hiding this comment

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

I guess everything will fail. But @kasperpeulen is working on adding metadata to tests, so I hope there's a way to configure this, either by adding the skip descrition or something, which allows us to get the results right

Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Agreed with Shilman's comments, but otherwise, this looks great to me! Nice work, Valentin!

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

I'm still a little suspicious about the documentation on how to configure the router to use globals. Otherwise the changes look great!

code/frameworks/nextjs/README.md Show resolved Hide resolved
@shilman shilman changed the title Support next/navigation in Next.js v13 NextJS: Support next/navigation in Next.js v13 Dec 7, 2022
@valentinpalkovic valentinpalkovic merged commit cc0c35f into next Dec 7, 2022
@valentinpalkovic valentinpalkovic deleted the valentin/support-next-navigation branch December 7, 2022 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants