-
-
Notifications
You must be signed in to change notification settings - Fork 829
Allow Chrome page translator to translate messages in rooms #11113
Allow Chrome page translator to translate messages in rooms #11113
Conversation
This is going to need a product review |
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.
Apologies for the long delay in reviewing this.
This is going to need a product review
First step of a review here is for someone to explain what this PR actually does, functionally speaking. @lukaszpolowczyk, please could you update the description of the PR to explain how this changes the user experience of the application. I see it links to an issue but there is a great deal of discussion in that issue and a concise summary would be useful
Also, please note that we require sign-off on all contributions. Please take a look at https://github.com/element-hq/element-web/blob/develop/CONTRIBUTING.md#sign-off and update the PR description with a "Signed-off-by" line.
Also, @SimonBrandner, if you have time: it would be good to record why you believe it needs product review? |
@SimonBrandner @SimonBrandner What else should I do? I want this PR to be pushed through. |
Sorry, I missed this, I am no longer working at Element... @richvdh, it probably ought to be checked if this is an expected behaviour for a chat app (I am not sure about that...), that is why I am suggesting a product review |
@SimonBrandner Eh, I don't understand this approach to the matter. |
I feel a little strawmanned - I do not wish to make anyone's life harder. I am not against this feature. What I am saying is: I do not know if this is expected behaviour for Element and such a thing was not up to me to decide when I was reviewing the PR, that is why I called for the product review. If @richvdh merges this, I am absolutely fine with it! |
@@ -399,6 +399,7 @@ export function bodyToHtml(content: IContent, highlights: Optional<string[]>, op | |||
"mx_EventTile_body": true, | |||
"mx_EventTile_bigEmoji": emojiBody, | |||
"markdown-body": isHtmlMessage && !emojiBody, | |||
"translate": true, |
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.
Please could you add a comment explaining a bit more about what this does, and why it is necessary? Surely it isn't normally necessary to mark things for translation explicitly?
Do I understand correctly that this is overriding a notranslate
class somewhere else? Please say that in a comment if so.
Basically: imagine you are a developer reading this code for the first time, without the benefit of having read all of the conversation on element-hq/element-web#25594, and that you are trying to understand what this is for.
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.
Do I understand correctly that this is overriding a notranslate class somewhere else? Please say that in a comment if so.
@richvdh Yes.
The entire application currently has translation disabled using the notranslate
class placed in the div#matrixchat
element.
The translate
class overrides the notranslate
class that is in this element's ancestor.
@SimonBrandner: thanks very much for the update. @lukaszpolowczyk: please be patient, nobody is trying to make life difficult unnecessarily. Anyway. @lukaszpolowczyk: Thanks for updating the PR summary as requested; that is helpful.
The element.io website is https://element.io/. I think you mean "the Element-Web application" (which is hosted at https://app.element.io, among many other places)? Remaining changes needed here:
Once you've made the necessary changes, do feel free to write a message here to remind us to look at them again. |
@richvdh I added Signed-off to commit. Should I add a more detailed commit somewhere else? Because I don't understand this: #11113 (comment) |
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.
@richvdh I added Signed-off to commit. But I will also add it to Pull Request.
Thank you. [Your commit does not include a Signed-off line. But that's fine, it is sufficient to put it in the PR].
Should I add a more detailed commit somewhere else? Because I don't understand this: #11113 (comment)
Please add a comment to the code.
…essage-translation-in-chat
fix element-hq/element-web#25594
The entire application currently has translation disabled using the
notranslate
class placed in thediv#matrixchat
element.The
translate
class overrides thenotranslate
class that is in this element's ancestor.This Pull request adds a
translate
class to each chat message. Thanks to this, the entire interface is still untouched by the Page Translator (like in Google Chrome), but specific messages, which are quite simple text, are translated.This increases accessibility for people who do not know a certain language.
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes