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

App router migration root layout #1085

Conversation

boodland
Copy link
Contributor

Parent PR for the progressive migration of _app.tsx and _document.tsx within the Root Layout

Issue link / number:

#1083

What changes did you make?

  1. Created basic root layout
  2. Added metadata to root layout
  3. Created public testing page to check migration

Why did you make the changes?

In order to be able to migrate the product from pages router to app router and benefits of all the new features available in the app router

Did you run tests?

Yes

Navigate to http://localhost:3000/public-route-test to check that title, charset and viewport meta tags are added properly

image

Copy link

vercel bot commented Aug 19, 2024

@boodland is attempting to deploy a commit to the Chayn Team on Vercel.

A member of the Team first needs to authorize it.

@boodland
Copy link
Contributor Author

@annarhughes I think better to create a app-router-migration branch so I can point all PR to that one, let me know when it is created to edit the PR so it points to app-router-migration

@boodland boodland force-pushed the app-router-migration-root-layout branch from c433cfb to d857356 Compare August 20, 2024 08:05
@annarhughes
Copy link
Member

Hey @boodland are you able to create that parent branch yourself, or even use this branch as the base/parent branch?
I'm not sure if you'd have the correct access if we were to create a branch from the origin repo

@boodland
Copy link
Contributor Author

Hey @boodland are you able to create that parent branch yourself, or even use this branch as the base/parent branch? I'm not sure if you'd have the correct access if we were to create a branch from the origin repo

Hi @annarhughes, I already tried to use this branch as the parent branch and all the PRs are created in my repo, not visible to Chayn and without Chayn's PR template. I don't think I am able to create a branch in Chayn repo, so please create one so I can point to and carry on with the development. Thanks!

@boodland boodland changed the base branch from develop to next-js-migration August 26, 2024 08:02
@boodland
Copy link
Contributor Author

@eleanorreem, @annarhughes I have just changed the base branch, have a look when possible. Thanks

Copy link

vercel bot commented Sep 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
bloom-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 10, 2024 0:18am

children: React.ReactNode;
}) {
return (
<html lang="en">
Copy link
Contributor

Choose a reason for hiding this comment

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

What the impact of setting lang="en" on a multi-lingual site? Might be worth making a TODO to address this later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once i18n is added the value will be set to locale. It is good to add a value for a11y reasons the issue is when the selected locale is not English (the default one) as not all the content within the site is translated to all available languages

@@ -1,5 +1,6 @@
/// <reference types="next" />
/// <reference types="next/image-types/global" />
/// <reference types="next/navigation-types/compat/navigation" />
Copy link
Contributor

@eleanorreem eleanorreem Sep 10, 2024

Choose a reason for hiding this comment

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

Hey @boodland what is this line doing? I saw this issue but not sure if its related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @eleanorreem, doesn't looks like as there isn't any empty folder within app one. It is automatically added when running yarn dev command, try to remove the line and run the command to see if it is also adding it for you, if it is the case, I would say is a reference that app router uses and needs to have in the definitions file

@boodland
Copy link
Contributor Author

@eleanorreem could you merge develop within nex-js-migration when possible in order to have the latest version? Thanks!

@eleanorreem eleanorreem merged commit 23afe00 into chaynHQ:next-js-migration Sep 11, 2024
5 checks passed
@eleanorreem
Copy link
Contributor

done! Thanks @boodland

@boodland boodland deleted the app-router-migration-root-layout branch September 12, 2024 09:49
@eleanorreem eleanorreem self-assigned this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants