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

1/4 Nextra v3 with i18n by locale folders #504

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

dimaMachina
Copy link
Contributor

@dimaMachina dimaMachina commented Sep 6, 2023

This PR includes migration to Nextra 3 with builtin support i18n by folders. More info about changes from v2 can be found in blogpost https://the-guild.dev/blog/nextra-3

also there is support of changing sidebar title via frontmatter prop

---
title: My page title
sidebarTitle: My sidebar title, otherwise fallback to frontmatter.title
---

@dimaMachina dimaMachina requested a review from a team as a code owner September 6, 2023 16:41
@github-actions

This comment was marked as off-topic.

@dimaMachina dimaMachina changed the title [wip] Nextra v3 with i18n by locale folders Nextra v3 with i18n by locale folders Jul 29, 2024
.node-version Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

why are we bringing these images in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

we were pulling these with remote-mdx in past so do we need to commit the files? see the same thing happing for graph-client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced remote mdx with catch all route [[..slug]].mdx to just fetch them, so in future they will be translated too

Comment on lines -22 to -25
uses: the-guild-org/shared-config/setup@main
with:
nodeVersion: 20
packageManager: pnpm
Copy link
Contributor

Choose a reason for hiding this comment

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

How does it know which version of Node to use if we don't specify it? (As for pnpm, I guess it either looks at which lockfile exists or at package.json's packageManager.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shared-config/setup@v1 looks packageManager field to understand which package manager and version to use.
and about Node.js it looks .node-version or .nvmrc file. @saihaj @enisdenjo correct me if I am wrong

Copy link
Member

Choose a reason for hiding this comment

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

shared-config/setup@v1 looks packageManager field to understand which package manager and version to use

Yes.

Node.js it looks .node-version or .nvmrc file

Not automatically, you have to provide either the node-version or node-version-file to the action.

.eslintrc.js Outdated Show resolved Hide resolved
.node-version Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
website/next.config.js Outdated Show resolved Hide resolved
Comment on lines 31 to 43
transformPageMap(pageMap) {
const locale = pageMap[0].data.slice(0, 2)
return [
...pageMap,
{
route: `/${locale}`,
name: 'index',
frontMatter: {
title: translate(translations, locale, 'index.title'),
},
},
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure I follow this. It seems to add a route for the [locale]/index.mdx file? Why is it needed for that particular route?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's a dynamic route, this is needed to translate sidebarTitle, both works title or sidebarTitle here but I'll rename to sidebarTitle to better understand

@@ -5,10 +5,10 @@ export default {
},
about: '',
network: 'The Graph Network',
sunrise: 'Sunrise Upgrade FAQ',
sunrise: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you moved "Sunrise Upgrade FAQ" out of this file into sunrise.mdx's sidebarTitle frontmatter, but why not do it for every page? Like "The Graph Network" just above?

Also, I guess the only purpose of these _meta.js files now is to set the order of the pages and add headings and separators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see you moved "Sunrise Upgrade FAQ" out of this file into sunrise.mdx's sidebarTitle frontmatter, but why not do it for every page? Like "The Graph Network" just above?

yep, I can do it in another PR

Also, I guess the only purpose of these _meta.js files now is to set the order of the pages and add headings and separators?

exactly

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should not be added; instead, any reference to it should be changed to match the existing filename (Delegation-Unbonding.png). I already fixed it in Crowdin, but we can update the sv MDX file manually until the Crowdin PR is merged (I will do it this week promise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I just added it temporarily until your PR is merged to the main

Copy link
Contributor

Choose a reason for hiding this comment

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

@dimaMachina – I just merged the Crowdin PR (#737) so this file and Indexing-Edward-Cut.png can be deleted. 👍


console.log(`✅ Remote files from "${url}" saved!`)
for (const fp of result.filePaths) {
const response = await fetch(`https://raw.githubusercontent.com/${user}/${repo}/main/${docsPath}${fp}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const response = await fetch(`https://raw.githubusercontent.com/${user}/${repo}/main/${docsPath}${fp}`)
const response = await fetch(`https://raw.githubusercontent.com/${user}/${repo}/${branch}/${docsPath}${fp}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice ☺️!

@dimaMachina dimaMachina changed the title Nextra v3 with i18n by locale folders 1/3 Nextra v3 with i18n by locale folders Jul 30, 2024
@dimaMachina dimaMachina changed the title 1/3 Nextra v3 with i18n by locale folders 1/4 Nextra v3 with i18n by locale folders Jul 30, 2024
* aa

* more

* lint fix

* 3/4 Refactor `_meta.js` to prefer `sidebarTitle` (#733)

* move mdxStyles

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* more

* lint

* update nextra

* 4/4 move `pages` to `src/pages` (#734)

* move pages to src

* some refactoring

* Update Node

* Remove unneeded `try…catch`

* `pnpm check:fix`

---------

Co-authored-by: benface <[email protected]>

---------

Co-authored-by: benface <[email protected]>
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.

4 participants