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

feat(guides): experimental i18n routing #5187

Merged
merged 22 commits into from
Nov 9, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Oct 24, 2023

Description (required)

This PR creates a guide page for internationalization APIs in Astro. I added "experimental" in the title, you will let me know what's best.

For Astro version: 3.5.0

Here's the RFC: withastro/roadmap#734
Draft PR: withastro/astro#8974

@netlify
Copy link

netlify bot commented Oct 24, 2023

Deploy Preview for astro-docs-2 ready!

Name Link
🔨 Latest commit dd4e916
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/654d3c9741d3df000875c3ef
😎 Deploy Preview https://deploy-preview-5187--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sarah11918
Copy link
Member

sarah11918 commented Oct 24, 2023

@ematipico So exciting to start to see docs here!

Just a reminder to please fill in the PR template when you can so I know which version this is for (like, should it be ready for tomorrow?? If unknown, you just leave it as 3.x as written, and that tells me it's unsure yet) and linking the implementation PR that adds the config options etc. so I have extra reference context when I start to look at it! Thanks!

@ematipico ematipico force-pushed the feat/experimental-i18n-routing branch 2 times, most recently from 42270e3 to 942311f Compare October 25, 2023 09:49
@sarah11918 sarah11918 added the merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) label Oct 29, 2023
@ematipico ematipico force-pushed the feat/experimental-i18n-routing branch from 8359722 to e26617d Compare November 2, 2023 12:50
@ematipico ematipico marked this pull request as ready for review November 2, 2023 12:50
@ematipico ematipico force-pushed the feat/experimental-i18n-routing branch from e26617d to a17a09d Compare November 2, 2023 12:51
@sarah11918 sarah11918 added the minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! label Nov 5, 2023
@ematipico
Copy link
Member Author

I updated the page with more information about the Fallback and the notes after the API bash session

@sarah11918
Copy link
Member

sarah11918 commented Nov 7, 2023

@ematipico HEADS UP! Have commited some structural changes:

  • renamed the file
  • created a sidebar entry for the page

Will continue to edit the content in this new situation!

itsmatteomanf

This comment was marked as outdated.

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

First pass!

Copy link
Contributor

@itsmatteomanf itsmatteomanf left a comment

Choose a reason for hiding this comment

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

Small typos... nothing major.

@sarah11918
Copy link
Member

Alright @ematipico! I will still need to do proofreading (there's at least one thing I've noticed!) but now's the time for you to fix anything that's incorrect! 😅

Thank you for providing all this wonderful content and examples!

As the feature develops, we will be updating here frequently so I'm mostly concerned with providing a good starting ground for people to start using the feature, and describing things in ways that people can really just put code in their project, see results and play around!

I think we have a nice user-facing page with a good flow here, and I think this is enough to let people easily get started working with the feature. But please do point out anything that might be incorrect, misleading or that would cause problems for a user. I think it's ok to not tell them everything here, as long as what is here is correct!

You can also specify a [`routingStrategy`](#routing-strategy) to customize the URLs for your default language.


2. Organize your content folders with localized content by language, including your `defaultLocale`. Your folder names must match the items in `locales` exactly. `src/pages/index.astro` will now automatically redirect to the `index.astro` file of your default language.
Copy link
Member Author

@ematipico ematipico Nov 8, 2023

Choose a reason for hiding this comment

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

The default routingStrategy is 'prefix-other-locales', which means that the defaultLocale mustn't be part of the URLs.

Hence, we should suggest a different file system structure:

- pages
  - index.astro
  - about.astro
  - es/
    - index.astro
    - about.astro
  - pt_BR/
    - index.astro
    - about.astro

As a result, we don't have to mention the redirect from / to /en.

Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify, (maybe this changed): I thought that all your FILES MUST ALWAYS be in a localized folder for both options. BUT, the URL routes generated only show the prefix for prefix-always.

So, it is also your FOLDER STRUCTURE that must be different for each different option?

Copy link
Member Author

@ematipico ematipico Nov 8, 2023

Choose a reason for hiding this comment

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

So, it is also your FOLDER STRUCTURE that must be different for each different option?

Yes, that's exactly right. We don't put restrictions on where the localised folders are and their names. The routing strategy helps get the URLs in check without doing too many redirects, it's the responsibility of the developer to have the physical pages in the correct folders.

This means that with routingStrategy: prefix-other-locales, a URL like this /en/blog will return a 404 (if en is the default locale).

I am happy to review the behaviour of the strategies as we go. After all, this is experimental. I am sorry for the confusion!

Copy link
Member

Choose a reason for hiding this comment

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

Got it! I can update to reflect this!


For example, if `i18n.defaultLocale: "en"` is configured:

- `src/pages/en/blog.astro` will produce the route `example.com/blog/`
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't correct. With the 'prefix-other-locales', developers must have src/pages/blog.astro in their file system, because it's "assumed" that pages outside of a lang/locale folder belong to the default locale. This is related to my comment above.

Comment on lines +202 to +205
- `Astro.preferredLocale`: Astro can compute a **preferred locale** for your visitor if their browser's preferred locale is included in your `locales` array. This value is undefined if no such match exists.


- `Astro.preferredLocaleList`: An array of all locales that are both requested by the browser and supported by your website. This produces a list of all compatible languages between your site and your visitor. The value is `[]` if none of the browser's requested languages are found in your `locales` array. If the browser does not specify any preferred languages, then this value will be [`i18n.locales`].
Copy link
Member Author

@ematipico ematipico Nov 8, 2023

Choose a reason for hiding this comment

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

I am not sure if it's worth mentioning it, but I leave the information here, and you decide: the locales coming from Accept-Language have their format, e.g. pt-BR, although it's possible that developers decide to use a different format inside i18n.locales, e.g. pt_BR, pt-br.

Astro under to hood does some transformations (no need to mention this) and eventually will return the format specified in i18n.locales.

So if Accept-Language is fr-CA;q=0.1, pt-BR;q=0.5", and i18n.locales is ["fr_CA", "pt_BR"]:

  • Astro.preferredLocale is "pt_BR";
  • Astro.preferredLocaleList is ["pt_BR", "fr_CA"], the order is important because it's kept from Accept-Language

Copy link
Member

Choose a reason for hiding this comment

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

Let me see what I can do here!

```
:::

3. When writing links to pages on your site, use a localized URL. You can write this URL directly, or compute it using the [`getRelativeLocaleURL()`](#getrelativelocaleurl) helper available from the [`astro:i18n` module](#virtual-module-astroi18n). This will always provide the correct, localized route and can help you correctly use, or check, URLs on your site.
Copy link
Member

Choose a reason for hiding this comment

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

When writing links to pages on your site, use a localized URL.

@ematipico Just checking on a case like:

  • prefix-other-locales so that a default locale URL route is example.com/blog/
  • you want to hardcode (not compute) the href and write it by hand
  • you write href="/blog/" ??? <-- if this is true, then I should NOT say write a localized URL

Is the above example correct? should I change the wording?

Copy link
Member Author

@ematipico ematipico Nov 8, 2023

Choose a reason for hiding this comment

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

you write href="/blog/" ??? <-- if this is true, then I should NOT say write a localized URL

That's correct. The URL /blog should be reflected in the file src/pages/blog/index.astro. With prefix-other-locales, it's assumed that the content outside of a localized folder belongs to the default locale.

Is the above example correct? should I change the wording?

I think so, we should mention "localized URL" only for those locales that aren't the default locale.

@sarah11918
Copy link
Member

OK @ematipico I took another swing at this one, with the new information, and I think I like where this page now ended up. Can you please read again for accuracy (it's a little bit changed) and let me know what you think?

Copy link
Member Author

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I can't approve my PR, but the structure looks good, and all the information seems correct to me. Thank you @sarah11918 for the awesome work! 💪

@sarah11918
Copy link
Member

Excellent, @ematipico , then I'll approve it for you! Thank you for your help with this! 🙌

@sarah11918
Copy link
Member

@itsmatteomanf we're testing and checking our /ptal command in Discord that shows the status of our reviews, and it looks like maybe you "requested changes" on this one and it's kind of messing the status of this PR there. Do you mind removing that, please?

(Heads up that we don't really use that feature in this repo, because too few people come back and remove them, and we have to chase down people to get PRs merged, so in future, please just always leave a review via a "comment" or "approval". Thanks!)

@itsmatteomanf
Copy link
Contributor

Sorry @sarah11918!

I assumed hiding the comment that was outdated already removed it. Should have approved now, so it should be fine...

Copy link
Member

@yanthomasdev yanthomasdev left a comment

Choose a reason for hiding this comment

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

A couple final nits, after these LGTM!

Co-authored-by: Yan Thomas <[email protected]>
@sarah11918 sarah11918 added the add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. label Nov 8, 2023
@sarah11918 sarah11918 merged commit ce35815 into main Nov 9, 2023
@sarah11918 sarah11918 deleted the feat/experimental-i18n-routing branch November 9, 2023 20:17
NinuzIBZ added a commit to NinuzIBZ/docs that referenced this pull request Feb 1, 2024
The file `nav.ts` has been updated to align with the English language documentation based on this commits withastro#4876 , withastro#5183 , withastro#5213 , withastro#5271 , withastro#5187 , withastro#4667 , withastro#5541 , withastro#5605 , withastro#5499 , withastro#6470 , withastro#6620
@NinuzIBZ NinuzIBZ mentioned this pull request Feb 1, 2024
yanthomasdev added a commit that referenced this pull request Feb 5, 2024
The file `nav.ts` has been updated to align with the English language documentation based on this commits #4876 , #5183 , #5213 , #5271 , #5187 , #4667 , #5541 , #5605 , #5499 , #6470 , #6620

Co-authored-by: Yan Thomas <[email protected]>
ArmandPhilippot added a commit to ArmandPhilippot/astro-docs that referenced this pull request Jun 28, 2024
* Translate "Internationalization (i18n) Routing"
(`guides/internationalization.mdx`) in French (added in withastro#5187)
yanthomasdev added a commit that referenced this pull request Jul 2, 2024
* Translate "Internationalization (i18n) Routing"
(`guides/internationalization.mdx`) in French (added in #5187)

Co-authored-by: Yan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants