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

fix(content-docs): translate generated index pagination title #8123

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

Conversation

Josh-Cena
Copy link
Collaborator

Pre-flight checklist

Motivation

This is the same kind of fix as #7634 — deferring the generation of some metadata until contentLoaded.

Test Plan

We don't have tests for i18n + metadata generation, but I did test locally:

Previous:

image

Current:

image

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Sep 21, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 21, 2022
@Josh-Cena
Copy link
Collaborator Author

@slorber I'm not sure if this qualifies as a breaking change since we removed some metadata returned from loadContent. But we should not anticipate piggy-backing the plugin as an intended use case, should we?

@netlify
Copy link

netlify bot commented Sep 21, 2022

[V2]

Name Link
🔨 Latest commit 80fe374
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6358d9f53e50080008ad47ec
😎 Deploy Preview https://deploy-preview-8123--docusaurus-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 settings.

@github-actions
Copy link

github-actions bot commented Sep 21, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 62 🟢 97 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 73 🟢 100 🟢 100 🟢 100 🟢 90 Report

@github-actions
Copy link

github-actions bot commented Sep 21, 2022

Size Change: 0 B

Total Size: 819 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 52.6 kB
website/build/assets/css/styles.********.css 112 kB
website/build/assets/js/main.********.js 614 kB
website/build/index.html 40.8 kB

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Sep 30, 2022

So what I understand is that we should add the navigation metadata to the docs only after having translated the sidebars (ie the category index titles).


What annoys be me a bit is that I wish the contentLoaded() was receiving data in an already "processed" shape where the data structure can immediately be used to construct routes and everything. IE make it easy for users to implement their own contentLoaded.

Maybe we need an extra processContent lifecycle for that? loadContent => translate => process > contentLoaded: does it make sense? IMHO we can delay this decision after having a first-class API to extend plugins.


Note: is this expected that the sidebar became untranslated? I'm assuming your local test didn't include downloading all the translated MD and you just translated the category index title locally?

CleanShot 2022-09-30 at 15 39 28@2x


@slorber I'm not sure if this qualifies as a breaking change since we removed some metadata returned from loadContent. But we should not anticipate piggy-backing the plugin as an intended use case, should we?

To me removing data from a lifecycle is a breaking change. But I wouldn't consider it a breaking change only if we added new data.

At the same time we don't officially provide an official public API to override contentLoaded so only users doing the temporary workaround suggested in #4138 would likely be affected by it. That may include some plugin authors.

I think we can mark it as a breaking change. It is a minor i18n bugfix that can wait 3.0 IMHO and it's not really worth it to backport it immediately.

@slorber
Copy link
Collaborator

slorber commented Sep 30, 2022

I guess you didn't update the snapshots for now to help the review, can you make the PR pass?

Also, we had the navigation snapshotted before, is there a good way to still prevent possible navigation regressions with our current test setup?

@Josh-Cena
Copy link
Collaborator Author

There are other tests failing because this has changed the loaded content. I will fix that later. And yes—I only hand-wrote the translation that needed to be written.

is there a good way to still prevent possible navigation regressions with our current test setup?

This is the reason I haven't fixed the tests yet. I think we can only snapshot the generated data instead.

@Josh-Cena Josh-Cena force-pushed the jc/fix-pagination-translation branch from 2630d28 to 0835d5f Compare October 26, 2022 06:03
@Josh-Cena
Copy link
Collaborator Author

OK! I think I've finally got the tests figured out.

@Josh-Cena Josh-Cena force-pushed the jc/fix-pagination-translation branch 2 times, most recently from deda01a to 543bcb8 Compare October 26, 2022 06:47
@Josh-Cena Josh-Cena force-pushed the jc/fix-pagination-translation branch from 543bcb8 to 80fe374 Compare October 26, 2022 06:55
@Josh-Cena
Copy link
Collaborator Author

Maybe not yet😅 Such a pain

@Lorde627

This comment was marked as off-topic.

@Lorde627
Copy link

any progress about this 👀

@ikallali
Copy link

ikallali commented Feb 3, 2024

+1

@yujingz
Copy link

yujingz commented Mar 7, 2024

Would much appreciate if we can take another look at this issue. This is esp. bad if EN is not the default locale.

@qvignaud
Copy link

We're encountering the same issue, and as @yujingz it feels really weird for users as the primary language of website is not EN.
Any news on it? And what could be done to help if required?

@ncoroy
Copy link

ncoroy commented Apr 8, 2024

Hi, we are experiencing the same issue with the generated index not being fully translated. Any plan to correct this shortly ? Is it possible to help in a way or another ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated category index page title not translated in pagination link
8 participants