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

Adds simple localization features #247

Merged
merged 5 commits into from
Oct 31, 2019

Conversation

bitschmidty
Copy link
Contributor

Changes:

  • on individual newsletter pages, show available translations, if they exist
  • filters on homepage for only english posts
  • filters on newsletter page for english posts
  • supports language additions
  • small fix to newsletter layout html

Notes:

  • translations will be located in _posts/[language-code]/newsletters/
  • only handles newsletter types for now
  • simple language codes (en, ja) for selecting translations. can consider flags or other visuals as well.

Testing:

@jnewbery
Copy link
Contributor

ACK 009fd1f. This seems fine for a low-key rollout of localized newsletters.

If we continue to get japanese and other language translations, we could:

  • add flags/graphics/drop-down menu to make this a bit more pleasing on the eye
  • add a localized https://bitcoinops.org/<code>/newsletters/ page so international readers have a place to find all newsletters translated to their language
  • add documentation/contrib scripts to make it easier for people to contribute translations.

Thanks for doing this @bitschmidty ! And thanks for handling everything with CG.

@bitschmidty
Copy link
Contributor Author

Rebased and pushed a commit which filters out non english posts from the existing feed.xml

We should also consider adding language specific feeds in the future.

@bitschmidty
Copy link
Contributor Author

Created #251 to track non blocking suggestions to localization in the future

@jnewbery
Copy link
Contributor

jnewbery commented Oct 30, 2019

Did you copy the base feed.xml from somewhere? (I would have guessed here: https://github.com/jekyll/jekyll-feed/blob/master/lib/jekyll-feed/feed.xml but it seems like there are some differences). If so, can you split add feed.xml template which filters for english only into two commits:

  • the first adds the unaltered feed.xml file, so reviewers can use it to generate the feed and verify that there are no differences from master.
  • the second makes your modifications to feed.xml

@bitschmidty
Copy link
Contributor Author

Split commits for the feed into two:

  1. template xml file from 0.10.0 of the jekyll-feed plugin (our current version)
  2. tweak to the template file for filtering on english language only

Copy link
Collaborator

@harding harding left a comment

Choose a reason for hiding this comment

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

Concept ACK. One bug (need to also filter /en/publications) and one style suggestion. Thanks, @bitschmidty!

Tested by builiding site with and without this PR as is, then diffing static HTML. Then doing that again with the suggested translation on top of this PR. Also browsed a local preview of the test site with the suggested translation. Reviewed code.

newsletters.md Show resolved Hide resolved
_layouts/newsletter.html Outdated Show resolved Hide resolved
@jnewbery
Copy link
Contributor

ACK 560573f.

Thanks for splitting out the feed change into two commits. I've tested the following:

  • with just the first commit (add jekyll-feed v 0.10.0 feed template file), there are only whitespace changes
  • with the first commit and sample japanese/other language posts, those posts are included in the feed
  • with the first commit, sample japanese/other language posts and the second commit (filter xml feed to english language only), the feed is the same as master except for whitespace. All non-english posts are filtered out.

Once you've addressed @harding's feedback, this is ready for merge.

@bitschmidty
Copy link
Contributor Author

Pushed 2 commits to address feedback from @jnewbery and @harding . Thanks for the review feedback as well as outlining your testing approaches!

Copy link
Collaborator

@harding harding left a comment

Choose a reason for hiding this comment

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

Tested ACK ba22ddd Thanks!

@jnewbery jnewbery added newsletters Publishing/translating/editing newsletters internationalization Making the site appropriate for multiple localizations labels Oct 31, 2019
@jnewbery
Copy link
Contributor

ACK ba22ddd

@jnewbery jnewbery force-pushed the 2019-10-localization branch from ba22ddd to f0b0377 Compare October 31, 2019 17:09
@jnewbery
Copy link
Contributor

I've squashed the two fixup commits into the relevant commits. Merging now.

Thanks for delivering this, @bitschmidty !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internationalization Making the site appropriate for multiple localizations newsletters Publishing/translating/editing newsletters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants