Skip to content
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

New Auto-Translator widget #399

Open
wants to merge 75 commits into
base: develop
Choose a base branch
from

Conversation

cmuldur
Copy link
Contributor

@cmuldur cmuldur commented Feb 3, 2025

This PR introduces a new widget called auto-translator

bzn-cp and others added 30 commits July 18, 2024 15:26
Accepting nested objects for translation inside payload object.
@tomaskikutis
Copy link
Member

Could you add a demo video on how the feature works? Do I need anything additional that is not in this repository to run it? Some API keys or environment variables?

@cmuldur
Copy link
Contributor Author

cmuldur commented Feb 17, 2025

Could you add a demo video on how the feature works? Do I need anything additional that is not in this repository to run it? Some API keys or environment variables?

The translator widget has 2 functionalities. One is to translate the other is to compare.
Translation can be directly applied to the authoring display.

Screen.Recording.2025-02-17.at.2.10.47.PM.mov

@chelsvdm
Copy link

the API keys / environment variables will be sent to Ana and Gideon through a 1Password link by Michal, they should hand this to you. thanks @tomaskikutis

@tomaskikutis
Copy link
Member

DeepL translation does work, but google one throws an error Translation error details: 'dict' object has no attribute 'expired'. Any idea why is that?

I was looking at the compare dialog and am not understanding what is it supposed to compare? AI translated text to manual translation? The only option in the dropdown I see is "current" and nothing else can be chosen.

Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

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

Hardcoded fields

The fields that are currently being translated are "headline", "headline_extended", "body_html". I would restructure code not to hardcode them, but put them in a list where it would work to configure more fields to be translated by simply adding a new field to the list.

CSS

The most significant issue is that you're using CSS classnames defined outside of this extension. We treat classnames as internal implementation details that can be freely refactored and long as ui-framework components do not break. Thus I suggest you use ui-framework components or components from other UI libraries or write custom CSS in this extesion(can be done by adding index.css file).

For layout, <Spacer> component from ui-framework should work for most use cases. By using it you wouldn't need custom CSS.

Translations

gettext is not used correctly in some places.

  • only pass inline strings to gettext - not variables. Otherwise translation string extraction does not work
  • do not concatenate the result of multiple gettext calls e.g. gettext("auto") + gettext("translate"). Use a single gettext instead - otherwise translators will not be able to translate properly without having access to the full phrase
  • not sure why capitalize is used everywhere. I would simply pass the string with desired capitalization to gettext

Other issues

  • usages of console log should be removed
  • avoid using @ts-ignore - for unused variables you can avoid the warning by prefixing variable name with an underscore

@txl-cp
Copy link

txl-cp commented Feb 20, 2025

DeepL translation does work, but google one throws an error Translation error details: 'dict' object has no attribute 'expired'. Any idea why is that?

I was looking at the compare dialog and am not understanding what is it supposed to compare? AI translated text to manual translation? The only option in the dropdown I see is "current" and nothing else can be chosen.

Our dev is saying that it's related to env variables not set up for the google translation api

this is a better demo of the comparison tool. The purpose is to see differences between the writethrus of an article
download

@txl-cp
Copy link

txl-cp commented Feb 21, 2025

Hardcoded fields

The fields that are currently being translated are "headline", "headline_extended", "body_html". I would restructure code not to hardcode them, but put them in a list where it would work to configure more fields to be translated by simply adding a new field to the list.

I've found it's more maintainable and explicit to list out form components like this. If the fields were to grow over time then gettext would need multiple ternary operators to get the correct translation based on some key. If requirements were to change on design/structure of the form then maintaining the list/map could get out of hand quickly with props/conditions/css. What do you think?

@tomaskikutis
Copy link
Member

If the fields were to grow over time then gettext would need multiple ternary operators to get the correct translation based on some key.

I would keep the translation together with field ID. There would be no ternaries required then to get the label.

const fields = [
    {
        id: 'headline',
        label: gettext('Headline'),
    },
];

If requirements were to change on design/structure of the form then maintaining the list/map could get out of hand quickly with props/conditions/css. What do you think?

I don't think so. If we were building this feature in the core, it would most likely be a requirement that users could choose which fields to translate from fields available in the content profile. If there are other cases of exceptional behavior, like where on the article the field belongs, I would also keep it that in the fields definition list and would make everything else run the same regardless which field is being translated.

Example of exceptional behavior in field definition:

const fields = [
    {
        id: 'headline',
        label: gettext('Headline'),
        get: (article) => article.headline,
        set: (article, value) => ({...article, headline: value}),
    },
    {
        id: 'headline_ext',
        label: gettext('Headline extended'),
        get: (article) => article.extra.headline_ext,
        set: (article, value) => ({...article, extra: {...article.extra, headline_ext: value}}),
    },
];

@txl-cp
Copy link

txl-cp commented Feb 24, 2025

Hardcoded fields

The fields that are currently being translated are "headline", "headline_extended", "body_html". I would restructure code not to hardcode them, but put them in a list where it would work to configure more fields to be translated by simply adding a new field to the list.

CSS

The most significant issue is that you're using CSS classnames defined outside of this extension. We treat classnames as internal implementation details that can be freely refactored and long as ui-framework components do not break. Thus I suggest you use ui-framework components or components from other UI libraries or write custom CSS in this extesion(can be done by adding index.css file).

For layout, <Spacer> component from ui-framework should work for most use cases. By using it you wouldn't need custom CSS.

Translations

gettext is not used correctly in some places.

* only pass inline strings to gettext - not variables. Otherwise translation string extraction does not work

* do not concatenate the result of multiple gettext calls e.g. gettext("auto") + gettext("translate"). Use a single gettext instead - otherwise translators will not be able to translate properly without having access to the full phrase

* not sure why capitalize is used everywhere. I would simply pass the string with desired capitalization to gettext

These have now been addressed. Please let me know if you see any issues with the latest commit.

Copy link
Member

@tomaskikutis tomaskikutis left a comment

Choose a reason for hiding this comment

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

Hardcoded fields were only partially addressed. See mentions of headline_extended. If it was fully dynamic there would only be 1 mention.

Regarding CSS - you should drop the helpers too. e.g. d-flex flex-wrap items-stretch content-stretch gap-1 - Spacer component should work fine for layout.

const TRANSLATION_LANGUAGES = {
en: {
value: "en",
getLabel: (gettext: ISuperdesk["localization"]["gettext"]) =>
Copy link
Member

Choose a reason for hiding this comment

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

you can directly import and call gettext in this file

{isOpen && (
<Container gap="large" direction="column" className="mx-2">
<div
className={`${WIDGET_ID}__compare-accordion-settings-container d-grid gap-2`}
Copy link
Member

Choose a reason for hiding this comment

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

avoid concatenating classnames, it is not possible to statically find usages then

"value" | "onChange"
> & { name: RecursiveKeyOf<T> & string };

export const FormTextEditorInput = <T,>({
Copy link
Member

Choose a reason for hiding this comment

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

no need for comma here

Copy link

Choose a reason for hiding this comment

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

needs comma here to prevent ambiguity between jsx vs generic for typescript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants