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.

Sign up for GitHub

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

Jump to bottom

Translator improvements #846

Merged
merged 7 commits into from Jan 3, 2017
Merged

Translator improvements #846

merged 7 commits into from Jan 3, 2017

Conversation

Undeadlol1
Copy link
Contributor

@Undeadlol1 Undeadlol1 commented Dec 13, 2016 •
edited

Adds:

  • stand alone translate() function which can translate anything outside of react context
  • user’s browser language detection and auto selection of language (currently disabled to avoid errors due to non-fixed translation files (it.js, fr.js, es.js)
  • fixes client_config
  • fixes locale data polyfills

@Undeadlol1 Undeadlol1 changed the title From golos to steemit incremental Translator improvements Dec 13, 2016
sneak
sneak reviewed Dec 13, 2016
View reviewed changes
server/server-html.jsx
@@ -50,7 +50,7 @@ export default function ServerHTML({ body, assets, locale, title, meta }) {
<link href="https://fonts.googleapis.com/css?family=Source+Serif+Pro:400,600" rel="stylesheet" type="text/css" />
{ assets.style.map((href, idx) =>
<link href={href} key={idx} rel="stylesheet" type="text/css" />) }
<script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=Intl.~locale.en"></script>
<script src="https://cdn.polyfill.io/v2/polyfill.min.js?features=Intl.~locale.en,Intl.~locale.ru,Intl.~locale.fr,Intl.~locale.es,Intl.~locale.it"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please find another solution for this; We will be removing all non-essential 3P javascript resources from steemit.com soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the polyfill script needs to be imported into our repo - that would be acceptable.

sneak
sneak previously requested changes Dec 13, 2016
View reviewed changes
Copy link
Contributor

@sneak sneak left a comment

Choose a reason for hiding this comment

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

No 3P javascript - find a replacement for polyfill

@Undeadlol1
Copy link
Contributor Author

This is a way suggested by react-intl wiki.
At the moment this is the only way known to me to support safari browser with react-intl

@roadscape roadscape mentioned this pull request Dec 22, 2016
Closed
6 tasks
@Undeadlol1
Copy link
Contributor Author

waiting for #862 to be approved

@sneak
Copy link
Contributor

sneak commented Dec 23, 2016

Thanks for keeping track. :) Things are a bit slow around here due to the holidays this week but I imagine that will get merged in soon.

@Undeadlol1
Copy link
Contributor Author

It's fine, there is no rush.
Happy holidays!

valzav
valzav approved these changes Dec 25, 2016
View reviewed changes
Copy link
Contributor

@valzav valzav left a comment

Choose a reason for hiding this comment

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

@roadscape or @jcalfee please review too. It looks good to me to merge.

@valzav valzav dismissed sneak’s stale review December 25, 2016 20:25

I removed 3p js reference

@sneak sneak closed this Jan 2, 2017
@roadscape roadscape changed the base branch from develop to master January 2, 2017 18:00
@roadscape roadscape reopened this Jan 2, 2017
@sneak
Copy link
Contributor

sneak commented Jan 2, 2017

heya @Undeadlol1 - could you please toss a dummy commit on the tip of your branch so that CircleCI will rerun the tests so we can merge this? For some reason it got stuck.

@Undeadlol1
Copy link
Contributor Author

Undeadlol1 commented Jan 2, 2017 •
edited

@sneak sure

@sneak
Copy link
Contributor

sneak commented Jan 2, 2017

Oh, that's what the problem was - it wasn't set to build forks. Can you do it one more time? I have the setting set in CircleCI now.

@sneak
Copy link
Contributor

sneak commented Jan 2, 2017

@Undeadlol1

@sneak
Copy link
Contributor

sneak commented Jan 2, 2017

You can just git commit --amend and change the message, then git push --force.

@Undeadlol1 Undeadlol1 force-pushed the from-golos-to-steemit-incremental branch from ff95fff to c5abc77 Compare January 2, 2017 20:41
@Undeadlol1
Copy link
Contributor Author

All checks passed. Yay!

@sneak
Copy link
Contributor

sneak commented Jan 2, 2017

Great news. @jcalfee @roadscape could one of you take a look, too? It looks good to me.

@roadscape roadscape added this to the 2017-01-08 - Week 1 milestone Jan 3, 2017
@roadscape
Copy link
Contributor

lgtm

@roadscape roadscape merged commit 27ae16a into steemit:master Jan 3, 2017
@sneak
Copy link
Contributor

sneak commented Jan 6, 2017

@Undeadlol1 thank you tremendously for your help with this - i18n is something we need to be good at. We're investigating using a service to allow better community translations. Thoughts?

@Undeadlol1
Copy link
Contributor Author

Undeadlol1 commented Jan 6, 2017 •
edited

@sneak my pleasure!
Community translations are a problem indeed.
There are two things which create problems:

  1. strings were changed. Example: join_steemit: 'Join Steemit' became join_APP_NAME: 'Join ' + APP_NAME
  2. strings were added
    We can document such changes with every release and wait for Pull Requests. This is not a problem for added strings because react-intl does a fallback if proper translation is was not found. But it's a problem for changed strings because they were not updated and can be missleading.
    Or we could create a chat room where this changes are announced and contributors can work on translations immediately.

Do not know about external services, have no experience using them. Can you give me a link to one?

roadscape added a commit that referenced this pull request Jan 10, 2017
@roadscape
restore a const which was removed in #846
2c81258
valzav pushed a commit that referenced this pull request Jan 11, 2017
@roadscape
restore a const which was removed in #846 (#972)
8b7e7f3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valzav valzav valzav approved these changes

@sneak sneak sneak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2017-01-08 - Week 1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Undeadlol1 @sneak @roadscape @valzav