search

propublica / facebook-political-ads

Monitoring Facebook Political Ads
MIT License
237 stars 50 forks source link

Improve Localisation #17

Closed tpreusse closed 7 years ago

tpreusse commented 7 years ago

Goals

  1. Ensure UI is in the selected locale
  2. Allow the user to read the terms of service in their own language

pac

Under the hood changes:

thejefflarson commented 7 years ago

On first glance, this looks great! I’ll take a deeper look and merge it in on Monday. Thanks!

On Sat, Nov 25, 2017 at 10:21 AM Thomas Preusse notifications@github.com wrote:

Goals

  1. Ensure UI is in the selected locale
  2. Allow the user to read the terms of service in their own language

[image: pac] https://user-images.githubusercontent.com/410211/33233316-ad8f71f4-d214-11e7-8e23-a6ab70d46e98.gif

Under the hood changes:

  • replace getMessage with a custom implementation it is not possible to set the locale with getMessage https://bugs.chromium.org/p/chromium/issues/detail?id=660704
  • adapt wording of language_instructions message I removed the Danish and Italian message for the time being (now falls back to the updated English message)
  • fix / add missing messages (terms_of_service_accept, language, country)

You can view, comment on, or merge this pull request online at:

https://github.com/propublica/facebook-political-ads/pull/17 Commit Summary

  • fix watch cmd
  • custom getMessage (replaces chrome.i18n.getMessage)
  • move country and language selection to the onboarding screen

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/propublica/facebook-political-ads/pull/17, or mute the thread https://github.com/notifications/unsubscribe-auth/AADYRdStiYcd_TpbJSqsB6rrPwVilj7iks5s6FqcgaJpZM4Qqguw .

tpreusse commented 7 years ago

Awesome.

I further improved the German and Swiss German translation and added a i18n & l10n section to the readme.

The toggle labels now look cleaner and should be more understandable in German:

screen shot 2017-11-26 at 18 12 02
tpreusse commented 7 years ago

I noticed that it loads country names in ['fr', 'fi', 'nl', 'sv'] which do not have translated messages: https://github.com/propublica/facebook-political-ads/pull/17/files#diff-31f285a1779d40d5c8d386d9addae424R12

I left them in. Maybe their countries should also be marked as active in i18n.js.

thejefflarson commented 7 years ago

I don't think we need to mark their countries as active just yet -- we don't have translations for them or news partners just yet so it would be a waste. I originally included them because there was some interest in joining up, so we'll just leave it as is.

I'm loving this pull request, a couple cool things I especially like:

This reduceRight trick is very cool.

And I didn't realize you could do stuff like this with connect but now that I see it in action it makes total sense.

Thanks you again for doing this! It really looks like a better user experience.

tpreusse commented 7 years ago

It was a pleasure. The repo setup is super smooth. I love the simplicity of the extension—makes it easy and very enjoyable to work on.

Great work, thank you.

Looking forward to seeing this in action.