Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Introduce I18n to Hacktoberfest #675

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ChaelCodes
Copy link

Description

starts #444

  • I added the i18n gem
  • application controller will pull locales off the URL and render actions with the locale
    • the default is still :en
    • the locale is sticky and will persist as the user navigates the site
    • the locale param is added as soon as a user clicks a link
    • This caused 2 specs to be updated to include the default locale
  • The closing homepage has had all static text removed and moved to en.yml
  • keys that are referenced in templates and do not exist for the locale config will error in development

One of the decisions I made was to pass locale via params - and to include the current locale in the default URL params. This could potentially set up a user expectation that they could modify the locale and see a translated version of the page, which is not fully implemented. I wonder if it'd be better to add that change once the site has been fully internationalized, and a few translations added.

Test process

  • When Hacktoberfest is in a closing state, go to /
  • There should be no translation errors, missing keys, or missing text
  • Click any link within the hacktoberfest site
  • ?locale=en is added to the URL
  • Click any link within the hacktoberfest site
  • ?locale=en stays on the URL
  • Go to ?locale=es
  • This will tell you that es is not a valid locale
  • If you make an es.yml file
  • It will complain about the first key on the page that is missing

Requirements to merge

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@ChaelCodes ChaelCodes marked this pull request as ready for review October 11, 2020 19:44
@MattIPv4 MattIPv4 self-requested a review October 11, 2020 19:46
@MattIPv4
Copy link
Member

Gonna label this as accepted so that it counts, just in case we don't get around to reviewing it in time. Tyvm for the PR!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants