-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature: Integrate Translator Service #52
Conversation
…mplicity sake on our side.
Pull Request Test Coverage Report for Build 11812040469Details
💛 - Coveralls |
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.
this looks great to me!
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.
This looks great overall, the separate folder for translate definitely keeps the modularity
var isVisible = $(this).closest('.sensitive-content-message').next('.translated-content').is(':visible'); | ||
if (isVisible) { | ||
$(this).text('Hide the translated message.'); | ||
} else { |
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 like it shows the translated message without hiding the original, seems like a good way to do it
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.
Very well organized changes and code. Maybe you could add comments to explain the code, but otherwise good job.
Added code to let the client request make a REST API request to the translator service.