-
Notifications
You must be signed in to change notification settings - Fork 50
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: Translate full topics #199
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import Component from "@glimmer/component"; | ||
import { tracked } from "@glimmer/tracking"; | ||
import { action } from "@ember/object"; | ||
import { service } from "@ember/service"; | ||
import DButton from "discourse/components/d-button"; | ||
import concatClass from "discourse/helpers/concat-class"; | ||
|
||
export default class ShowOriginalContent extends Component { | ||
@service router; | ||
@tracked active = true; | ||
|
||
constructor() { | ||
super(...arguments); | ||
this.active = !new URLSearchParams(window.location.search).has("show"); | ||
} | ||
|
||
@action | ||
async showOriginal() { | ||
this.active = !this.active; | ||
window.location.search = this.active ? "" : `show=original`; | ||
} | ||
|
||
get title() { | ||
return this.active | ||
? "translator.hide_translation" | ||
: "translator.show_translation"; | ||
} | ||
|
||
<template> | ||
<div class="discourse-translator_toggle-original"> | ||
<DButton | ||
@icon="language" | ||
@title={{this.title}} | ||
class={{concatClass "btn btn-default" (if this.active "active")}} | ||
@action={{this.showOriginal}} | ||
/> | ||
</div> | ||
</template> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import { withPluginApi } from "discourse/lib/plugin-api"; | |
import { i18n } from "discourse-i18n"; | ||
import LanguageSwitcher from "../components/language-switcher"; | ||
import ToggleTranslationButton from "../components/post-menu/toggle-translation-button"; | ||
import ShowOriginalContent from "../components/show-original-content"; | ||
import TranslatedPost from "../components/translated-post"; | ||
|
||
function initializeTranslation(api) { | ||
|
@@ -22,8 +23,9 @@ function initializeTranslation(api) { | |
{ before: ["search"] } | ||
); | ||
} | ||
|
||
if (currentUser) { | ||
if (siteSettings.experimental_topic_translation) { | ||
api.renderInOutlet("topic-navigation", ShowOriginalContent); | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we should still check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean here? This feature will be available for anons also. Do you mean we should disable it "if anon + no language switcher"? Ok, that makes sense. |
||
customizePostMenu(api); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,3 +2,19 @@ | |||||
.fk-d-menu__inner-content { | ||||||
max-height: 50vh; | ||||||
} | ||||||
|
||||||
.topic-navigation.with-timeline .discourse-translator_toggle-original { | ||||||
margin-bottom: 0.5em; | ||||||
} | ||||||
|
||||||
.topic-navigation.with-topic-progress | ||||||
.discourse-translator_toggle-original | ||||||
button { | ||||||
height: 100%; | ||||||
} | ||||||
|
||||||
.discourse-translator_toggle-original { | ||||||
button.active svg { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We usually target the icon class. Shouldn't active buttons be using the correct color in core? |
||||||
color: var(--tertiary); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
|
||
module DiscourseTranslator | ||
module TranslatorHelper | ||
def self.translated_value(original_value, model, scope) | ||
return original_value if !SiteSetting.experimental_topic_translation | ||
return original_value if scope.request.params["show"] == "original" | ||
|
||
translated = model.custom_fields[TRANSLATED_CUSTOM_FIELD] | ||
return original_value if (translated.blank? || translated[I18n.locale].blank?) | ||
|
||
translated[I18n.locale] | ||
Comment on lines
+9
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we are ready to solve this yet but if the translation is missing, do we display any indication in the interface for the user? It would be odd to the user if they request for a translation and we return the original one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will come in a subsequent commit. The idea is that in the future, all topics will be translated by default. There needs to be a new outlet in core for the "indicator" on both the topic posts itself and the topic list view titles. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,4 +47,13 @@ module ::DiscourseTranslator | |
add_to_serializer :post, :can_translate do | ||
scope.can_translate?(object) | ||
end | ||
|
||
add_to_serializer :post, :cooked, respect_plugin_enabled: false do | ||
return super() if cooked_hidden | ||
DiscourseTranslator::TranslatorHelper.translated_value(super(), object, scope) | ||
end | ||
Comment on lines
+51
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overriding a core's serializer attribute is going to be fragile to maintain long term. Perhaps we need an API extension in core? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tgxworld We discussed this internally, can take a look at t/146193/5 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've read through the post but it doesn't address my concern about us overriding a core's serializer attribute from a plugin which is a form of monkey patching. Just to clarify, my concern here is not about us changing the value of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can revert to 2️⃣ in t/146193/1 if that helps. But the new 'problem' is that we'll be transmitting a lot more over the wire (translated_cooked+cooked) for something a user might not use (cooked) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have strong opinions on whether 1 or 2 is the better approach. What I am saying is that if we are going with 1, we probably want to introduce a plugin modifier/API in Discourse core that allows us to modify the value of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to have built-in protection for the existing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is experimental, I think we can add a note above the two |
||
|
||
add_to_serializer :basic_topic, :fancy_title do | ||
DiscourseTranslator::TranslatorHelper.translated_value(object.fancy_title, object, scope) | ||
end | ||
Comment on lines
+56
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one doesn't have an easy decorator unlike post.cooked.. 🤔 |
||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rails_helper" | ||
|
||
describe BasicTopicSerializer do | ||
let!(:guardian) { Guardian.new(user) } | ||
let!(:original_title) { "FUS ROH DAAHHH" } | ||
let!(:jap_title) { "フス・ロ・ダ・ア" } | ||
|
||
describe "#fancy_title" do | ||
fab!(:user) { Fabricate(:user, locale: "ja") } | ||
fab!(:topic) | ||
|
||
before do | ||
topic.title = original_title | ||
SiteSetting.experimental_topic_translation = true | ||
I18n.locale = "ja" | ||
end | ||
|
||
def serialize_topic(guardian_user: user, params: {}) | ||
env = { "action_dispatch.request.parameters" => params, "REQUEST_METHOD" => "GET" } | ||
request = ActionDispatch::Request.new(env) | ||
guardian = Guardian.new(guardian_user, request) | ||
BasicTopicSerializer.new(topic, scope: guardian) | ||
end | ||
|
||
it "returns original fancy_title when experimental_topic_translation is disabled" do | ||
SiteSetting.experimental_topic_translation = false | ||
topic.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] = { "ja" => jap_title } | ||
|
||
expect(serialize_topic.fancy_title).to eq(original_title) | ||
end | ||
|
||
it "returns original fancy_title when show_original param is present" do | ||
topic.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] = { "ja" => jap_title } | ||
expect(serialize_topic(params: { "show" => "original" }).fancy_title).to eq(original_title) | ||
end | ||
|
||
it "returns original fancy_title when no translation exists" do | ||
expect(serialize_topic.fancy_title).to eq(original_title) | ||
end | ||
|
||
it "returns original fancy_title when translation is blank for current locale" do | ||
topic.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] = { "ja" => "" } | ||
expect(serialize_topic.fancy_title).to eq(original_title) | ||
end | ||
|
||
it "returns translated title when translation exists for current locale" do | ||
topic.custom_fields[DiscourseTranslator::TRANSLATED_CUSTOM_FIELD] = { "ja" => jap_title } | ||
expect(serialize_topic.fancy_title).to eq(jap_title) | ||
end | ||
end | ||
end |
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.
Is it possible that there will be params already? Maybe it would be safer to append params?
https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/append