-
Notifications
You must be signed in to change notification settings - Fork 18
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
Multiple target languages #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest looks good to me. You should also run npm run format:fix
and npm run lint:fix
on your code for linting/autoformat.
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
Updating changes from feedback given
…-node into multiple_target_languages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall, just a few nits.
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
Looks good from my side. Only thing left is to squash all the commits into one, and adhere to our conventional commit naming scheme. Probably makes sense to add this to the |
examples/multiple-target-languages/MultipleLanguagesIterator.js
Outdated
Show resolved
Hide resolved
646549b
to
47138a9
Compare
47138a9
to
9c12bd8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We can merge this once the other threads are resolved.
Changes approved here #45 (comment)
Showing users how to translate a website into multiple languages in one go.