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

Move custom translation strings to its own file #775

Merged
merged 3 commits into from
Jun 15, 2018

Conversation

JoelMarcey
Copy link
Contributor

@JoelMarcey JoelMarcey commented Jun 14, 2018

Motivation

Allowing custom and overriding translation strings in en.json caused us to not delete en.json like we used to do and thus would not allow updates to header to be reflected any longer.

Fixes #713

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Local testing

screenshot 2018-06-14 15 52 09

Related PRs

#158
#710

@JoelMarcey
Copy link
Contributor Author

This is still a work in progress. Got some more testing and changes I want to do, but in my initial testing, this seems to fix the problem.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 14, 2018
@JoelMarcey JoelMarcey requested review from yangshun and endiliey June 14, 2018 17:10
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit ab1ca99

https://deploy-preview-775--docusaurus-preview.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 14, 2018

Deploy preview for docusaurus-preview ready!

Built with commit efc6f06

https://deploy-preview-775--docusaurus-preview.netlify.com

@endiliey
Copy link
Contributor

  1. Do we want to let user to define their own path to the custom translation string or must it be hardcoded /website/data/custom-translation-strings.json ?
  2. Update the docs 😀

@JoelMarcey
Copy link
Contributor Author

Do we want to let user to define their own path to the custom translation string or must it be hardcoded /website/data/custom-translation-strings.json ?

Given that we already have a data directory for custom information (e.g. site users), I think defaulting to that directory is reasonable. If we get pushback for a custom siteConfig.js option, we can always add it in the future -- but trying to avoid adding more to siteConfig.js unless there is a big want/need.

Seem reasonable?

Update the docs 😀

Absolutely! Just making sure this is the right direction. 😄

@endiliey
Copy link
Contributor

Seems reasonable for me. Let's not bloat siteConfig.js 🤣

LGTM. Seems to be in the right direction

@JoelMarcey
Copy link
Contributor Author

@endiliey Docs - https://deploy-preview-775--docusaurus-preview.netlify.com/docs/en/next/translation#custom-translation-strings

I am just going to show a test plan, then I will remove the [RFC] from this and get it officially reviewed and merged.

@JoelMarcey JoelMarcey requested a review from rickyvetter June 14, 2018 18:15
@rickyvetter
Copy link
Contributor

Looks awesome! Thanks for fixing :)

@vaibhavsingh97
Copy link

@JoelMarcey This looks awesome to me 👍

@JoelMarcey JoelMarcey changed the title [RFC] Move custom translation strings to its own file Move custom translation strings to its own file Jun 14, 2018
@JoelMarcey
Copy link
Contributor Author

Added a test plan. This should be ready for review and merging.

@JoelMarcey JoelMarcey added the proposal This issue is a proposal, usually non-trivial change label Jun 14, 2018
@JoelMarcey JoelMarcey merged commit 608e2c8 into facebook:master Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA proposal This issue is a proposal, usually non-trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants