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(i18n): add i18next keys for ShowProjectLinks #41239

Conversation

errmayank
Copy link
Contributor

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

Closes #41155

  • The translation.json file for Chinese and Spanish uses English values for now.
  • I've used the project title as a key in translations.json, certification => project => title => 'Project Title' to translate project titles let me know if there's a better way to do this.
  • In ShowProjectLinks.js I've used the t function to translate simple texts and for more complex (nested) cases I've used the <Trans /> component.

@errmayank errmayank requested a review from a team February 23, 2021 19:43
@gitpod-io
Copy link

gitpod-io bot commented Feb 23, 2021

@moT01
Copy link
Member

moT01 commented Feb 23, 2021

I believe these title are already translated in the challenge files - probably be pretty tough to pull them from there though.

I guess they should get translated automatically by crowdin I think, since the strings will match what has already been translated. Just thinking out loud here.

@ojeytonwilliams
Copy link
Contributor

I believe these title are already translated in the challenge files - probably be pretty tough to pull them from there though.

That's definitely how it should be done. Then there would be no need to use Trans for the titles as they'd already be translated. That said, it's not trivial, so it may take a while to implement.

@RandellDawson how much extra work are we generating by temporarily changing translations.json?

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Hi @errmayank nice work on this PR. I have a few suggestions on how it could be simplified slightly:

client/src/client-only-routes/ShowProjectLinks.js Outdated Show resolved Hide resolved
client/src/client-only-routes/ShowProjectLinks.js Outdated Show resolved Hide resolved
client/src/client-only-routes/ShowProjectLinks.js Outdated Show resolved Hide resolved
client/src/client-only-routes/ShowProjectLinks.js Outdated Show resolved Hide resolved
@errmayank errmayank force-pushed the fix/i18n-keys-for-showprojectlinks branch from eeaff05 to f40c6fa Compare February 24, 2021 18:44
@errmayank
Copy link
Contributor Author

@ojeytonwilliams Thanks for the help! I've made the necessary changes.

@raisedadead raisedadead added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label Feb 25, 2021
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Hey @errmayank thanks again for your hard work on this.

As well as the suggestions below, could you revert the changes to /chinese/translations.json and /espanol/translations.json? We just changed our approach so we default to English #41246 which means we're better off leaving those files alone and relying on the defaults.

client/i18n/locales/english/translations.json Outdated Show resolved Hide resolved
client/src/client-only-routes/ShowProjectLinks.js Outdated Show resolved Hide resolved
@errmayank
Copy link
Contributor Author

@ojeytonwilliams cool! just to make sure, I should keep /english/translations.json as it is right now with project titles right?

@errmayank
Copy link
Contributor Author

errmayank commented Feb 25, 2021

@ojeytonwilliams weird but after changing the footnote to what you suggested and adding those default values, the text does not render properly

Screenshot from 2021-02-26 01-55-12

@errmayank
Copy link
Contributor Author

Here's how I was able to make this work by using fragments

"footnote": "If you suspect that any of these projects violate the <1>academic honesty policy</1>, please <3>report this to our team</3>."
<Trans i18nKey='certification.project.footnote'>
  <>If you suspect that any of these projects violate the </>
  <a
    href='https://www.freecodecamp.org/news/academic-honesty-policy/'
    target='_blank'
    rel='noreferrer'
  >
    academic honesty policy
  </a>
  <>, please </>
  <a
    href={`/user/${username}/report-user`}
    target='_blank'
    rel='noreferrer'
  >
    report this to our team
  </a>
  <>.</>
</Trans>

@ojeytonwilliams
Copy link
Contributor

cool! just to make sure, I should keep /english/translations.json as it is right now with project titles right?

Yes, that's exactly right, thanks.

Here's how I was able to make this work by using fragments

Nice job getting that to work, but we can get away without using fragments here. The trick is to find the correct numbers for the translation string and that's explained here. Long story short: we have to look at the children of the <Trans> component to find those numbers.

@ojeytonwilliams
Copy link
Contributor

I've updated my suggestion - if that doesn't work for you, just let me know and I'll make 100% it works locally and push a commit!

@moT01
Copy link
Member

moT01 commented Feb 26, 2021

Random component from the landing page:

Screen Shot 2021-02-25 at 6 28 27 PM

You can use the react dev tools to see the children - as it states in the linked docs. Nice to get a visualization some times.

CyberFlameGO
CyberFlameGO previously approved these changes Feb 27, 2021
@errmayank
Copy link
Contributor Author

@ojeytonwilliams great! the updated replacement tags that you suggested is working, I've pushed my changes.

@errmayank
Copy link
Contributor Author

The Lint Source Files test failed due to missing keys in chinese and espanol translations.json

@RandellDawson
Copy link
Member

@ojeytonwilliams I thought we had changed it so we no longer have to add the non-English keys. Instead, the fallback was English and then when the English source file gets uploaded to Crowdin gets uploaded, the file would get the English version at first until translated/approved.

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

@RandellDawson so did I, but it looks like we just agreed that that was the plan.

@errmayank thanks again for bearing with us on this. As far as I'm concerned this PR is done, so I'll take care of the validation problem myself and rebase this PR once that's complete.

@RandellDawson
Copy link
Member

@raisedadead Can you confirm that we still want to have to add changes to the non-English files for translations.json? I thought we were going to stick with using English as the fallback for any missing keys, then once Crowdin downloads the translations, the translations for the key values would be used.

@naomi-lgbt
Copy link
Member

English is currently set to be the fallback value - if we choose to go this direction, I can disable the linting specifically for the intro.json and translation.json files.

@raisedadead
Copy link
Member

Yes - @nhcarrigan you can go ahead and remove the linting. Maybe we can downgrade that to a warning or something just as an FYI, but it should not throw.

@errmayank
Copy link
Contributor Author

I guess instead of throw new Error() in schema-validation.js#L135 we could just do a console.warn()?

@ojeytonwilliams
Copy link
Contributor

GitHub actions are currently down, so we may need to restart them once that is resolved.

@naomi-lgbt
Copy link
Member

Looks to be resolved now, according to their status page.

@naomi-lgbt naomi-lgbt closed this Mar 1, 2021
@naomi-lgbt naomi-lgbt reopened this Mar 1, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 1, 2021

Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ojeytonwilliams ojeytonwilliams changed the title Add i18next keys for ShowProjectLinks component fix(i18n): add i18next keys for ShowProjectLinks Mar 1, 2021
@ojeytonwilliams ojeytonwilliams merged commit 01a7a8c into freeCodeCamp:main Mar 1, 2021
@ojeytonwilliams
Copy link
Contributor

@errmayank Congrats on your first pull request (PR)! 🎉

Thank you for your contribution to the page! 👍
We're happy to accept these changes, and look forward to future contributions. 📝

@errmayank errmayank deleted the fix/i18n-keys-for-showprojectlinks branch March 1, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add i18n keys to ShowProjectLinks component
8 participants