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: remove algolia indices and urls from translations #41043

Conversation

scissorsneedfoodtoo
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 #41029

This PR moves the Algolia index names and News search page URLs out of the translation.json files to prevent accidental changes on CrowdIn.

It also adds some basic tests for the search bar.

@scissorsneedfoodtoo scissorsneedfoodtoo requested a review from a team February 10, 2021 15:28
@gitpod-io
Copy link

gitpod-io bot commented Feb 10, 2021

@scissorsneedfoodtoo scissorsneedfoodtoo force-pushed the fix/remove-algolia-indices-and-urls-from-translations branch from c699b2d to d7b5699 Compare February 10, 2021 16:52
Copy link
Member

@raisedadead raisedadead left a comment

Choose a reason for hiding this comment

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

LGTM.

@RandellDawson This would be a good test of what happens we delete stuff on a translated file both the source and translation.

@RandellDawson
Copy link
Member

@raisedadead @nhcarrigan and I have a plan.

  1. Merge any PR with changes to the English source (when they are ready)
  2. Upload the English source via the normal Crowdin upload workflow
  3. I will manually add all the translations that were made two the intros.json and translations.json (Nick will approve)
  4. Run the download for these files (after a small tweak to the config to allow both files to be downloaded but only approved strings come down).

This is the easiest way to bring us back into sync. Hopefully, we will not be making any further changes to the non-English versions of these files outside of Crowdin in the future.

@raisedadead raisedadead merged commit 1e9dc8c into freeCodeCamp:main Feb 10, 2021
@scissorsneedfoodtoo scissorsneedfoodtoo deleted the fix/remove-algolia-indices-and-urls-from-translations branch February 11, 2021 03:45
@scissorsneedfoodtoo
Copy link
Contributor Author

@RandellDawson, looking at the translations.json file, it seems like there are about 12 links there. I think 11 of them are related to the footer, and one is for an error message.

Would it be better to extract those footer links from the translations.json file and merge them with the ones in trending.json? We'd probably have to rename trending.json to something like footer-links.json, too. That's assuming we'll handle all the footer links and text localization in house.

I'm thinking this might be the way to go because we've had some issues with text overflow on News, and need to make changes before updating the theme.

If we'll handle those on Crowdin as well, is it possible to hide those links so they're not accidentally changed?

@RandellDawson
Copy link
Member

RandellDawson commented Feb 11, 2021

If we'll handle those on Crowdin as well, is it possible to hide those links so they're not accidentally changed?

@scissorsneedfoodtoo We can hide any string in the JSON files manually or create an action to hide specific keys' values by key name (i.e. footer.links.about-url). I count 10 urls as stand-along values in the translations.json file. 9 start with footer.links and one is donate.other-ways-url.

I would leave them in translations.json. We could hide anything that had a key ending in -url.

@scissorsneedfoodtoo
Copy link
Contributor Author

@RandellDawson, awesome, that sounds like a plan. Looking over the file again we shouldn't need to remove any other keys, at least as far as I can tell.

@ojeytonwilliams
Copy link
Contributor

Let's not mix translatable and untranslatable content in one file if we can avoid it. We already have files like trending.json for content that differs between the languages, but does not want translating. I think we should do the same here.

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.

Algolia 404 error showing in console when Spanish home page loads.
4 participants