Skip to content

Conversation

@marcoambrosini
Copy link
Member

@marcoambrosini marcoambrosini commented May 10, 2023

☑️ Resolves

Requisites

🖼️ Screenshots

image
image

🚧 Tasks

  • Code review
  • Visual check
  • Handle rich content properly

🏁 Checklist

@nickvergessen nickvergessen added this to the 💙 Next Beta (27) milestone May 11, 2023
@nickvergessen nickvergessen mentioned this pull request May 11, 2023
4 tasks
@marcoambrosini marcoambrosini force-pushed the feature/9273/translate-chat-messages branch from 8b15247 to 500de38 Compare May 11, 2023 12:48
@marcoambrosini marcoambrosini marked this pull request as ready for review May 11, 2023 13:14
@marcoambrosini marcoambrosini force-pushed the feature/9273/translate-chat-messages branch from 500de38 to 120eaae Compare May 11, 2023 13:16
@Antreesy
Copy link
Contributor

Antreesy commented May 11, 2023

So, i get a successful responce on axios.get(generateOcsUrl('/translation/languages')):
{"ocs":{"meta":{"status":"ok","statuscode":200,"message":"OK"},"data":{"languages":[{"from":"de","fromLabel":"German","to":"en","toLabel":"English"},{"from":"en","fromLabel":"English","to":"de","toLabel":"German"}],"languageDetection":true}}}

But when trying to translate with axios.post(generateOcsUrl('/translation/translate'), get an error:
{"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":{"message":"Unable to translate","from":"en"}}}

@Antreesy Antreesy force-pushed the feature/9273/translate-chat-messages branch from 120eaae to 8497bd7 Compare May 12, 2023 13:18
@Antreesy Antreesy requested a review from nickvergessen May 12, 2023 13:18
@Antreesy Antreesy changed the title Feature/9273/translate chat messages Translate chat messages May 12, 2023
@nimishavijay
Copy link
Member

I messed around a bit more with how this could look and I'd like to propose one more design change: use a border instead of a background color.

  • Give --border-radius-large border radius to both the texts
  • Use a --color-border border for the source text
  • Use a --color-primary-element border for the target text
  • use --color-main-background background for both the texts

what do you think? :)

@Antreesy
Copy link
Contributor

what do you think? :)

Looks good and a bit less as Google Translate) Also user mentions are visible better now
image

@Antreesy Antreesy requested a review from nickvergessen May 12, 2023 15:41
@Antreesy Antreesy enabled auto-merge May 12, 2023 16:03
Comment on lines +369 to +372
isTranslationAvailable: {
type: Boolean,
required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the isTranslationAvailable always supposed to have value from capabilities?
If it is, maybe use capability instead of prop here? Or at least use capabilities as the default value. Then the long list of props to pass will be a little bit less 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Could be done later, when we'll finally reach Message refactoring =)

Comment on lines +223 to +234
async translateMessage(sourceLanguage = null) {
try {
this.isLoading = true
const response = await translateText(this.message, sourceLanguage, this.selectedTo?.id)
this.translatedMessage = response.data.ocs.data.text
} catch (error) {
console.error(error)
showError(error.response?.data?.ocs?.data?.message ?? t('spreed', 'The message could not be translated'))
} finally {
this.isLoading = false
}
},
Copy link
Contributor

@ShGKme ShGKme May 12, 2023

Choose a reason for hiding this comment

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

On the first modal open it always fails to me from the request in mounted:

Request:

fetch("https://<HOST>/ocs/v2.php/translation/translate", {
  "headers": {
    "accept": "application/json, text/plain, */*",
    "accept-language": "en-US,en;q=0.9,ru;q=0.8",
    "content-type": "application/json",
    "requesttoken": "<DELETED>",
    "sec-ch-ua": "\"Chromium\";v=\"112\", \"Not:A-Brand\";v=\"99\"",
    "sec-ch-ua-mobile": "?0",
    "sec-ch-ua-platform": "\"Windows\"",
    "sec-fetch-dest": "empty",
    "sec-fetch-mode": "cors",
    "sec-fetch-site": "same-origin"
  },
  "referrerPolicy": "no-referrer",
  "body": "{\"text\":\"Hallo!\",\"fromLanguage\":null,\"toLanguage\":\"en\"}",
  "method": "POST",
  "mode": "cors",
  "credentials": "include"
});

Response:

{"ocs":{"meta":{"status":"failure","statuscode":400,"message":""},"data":{"message":"Could not detect language"}}}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's on API side then, we always send text and 'to' language

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it seems, API is used correctly

Copy link
Member

Choose a reason for hiding this comment

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

That's what I said in the office. You send a single word and fromLanguage is null.
But on a single word language models don't work and respond with a language, so we don't have a from when talking to translations provider and it returns with could not translate

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should not show the error message in this case?
Otherwise, all small messages show an error toast on opening the translation modal.
Also, if only one language is available, we can pre-select it.

Copy link
Member

Choose a reason for hiding this comment

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

Well we could try to hide the error from empty runs yeah, but let's do that in a follow up

@Antreesy Antreesy merged commit 358d043 into master May 12, 2023
@Antreesy Antreesy deleted the feature/9273/translate-chat-messages branch May 12, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Translate chat messages [web frontend]

6 participants