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

feat: add translation context for select options label #26352

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

Conversation

FHenry
Copy link
Contributor

@FHenry FHenry commented May 7, 2024

Please provide enough information so that others can review your pull request:

On input type select the options value may need context for translation.
For example in Letter Head field align is a select as select Left/Right/....
in French/English
Left can be translated as "Left"(en) => "Parti"(fr) : the preterit of the verb "to leave"
Left can be translated as "Left"(en) => "Gauche"(fr) : the direction
I can't just put french translation for "Left,Gauche" because in other Doctype "Left" really mean "to leave"
I suggest to have a contextual translation options here

Explain the details for making this change. What existing problem does the pull request solve?

send dt and df in parse_option to allow __(xx,null,context) top be build correctly according the context

no-docs

@FHenry FHenry requested a review from a team as a code owner May 7, 2024 07:09
@FHenry FHenry requested review from akhilnarang and removed request for a team May 7, 2024 07:09
Copy link
Contributor

@cogk cogk left a comment

Choose a reason for hiding this comment

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

Let's make sure that variable names make sense in the future 😄

df is often used for the docfield object, not just it's name. So this could be confusing

frappe/public/js/frappe/form/controls/select.js Outdated Show resolved Hide resolved
frappe/public/js/frappe/form/controls/select.js Outdated Show resolved Hide resolved
frappe/public/js/frappe/form/controls/select.js Outdated Show resolved Hide resolved
@FHenry FHenry force-pushed the dev_add_translation_context_for_select_option branch from 43997df to a5f5cfa Compare May 13, 2024 15:32
@FHenry
Copy link
Contributor Author

FHenry commented May 20, 2024

@barredterra Don't you have the same issue with German translation ?

@barredterra
Copy link
Collaborator

barredterra commented May 20, 2024

Currently the code generating the translation files only allows for two context types:

  1. arbitrary string literals used as parameter to the translation function (no variables allowed)
  2. the DocType name (can be a variable)

So, if you change your code to only use the DocType as context, this should be good to go.

See also: #24903

@FHenry
Copy link
Contributor Author

FHenry commented May 21, 2024

@barredterra This is not the only case where context is "Typed/field" named

duration += total_duration.days + __("d", null, "Days (Field: Duration)");

Here there is type of the field and another information for the context

@barredterra
Copy link
Collaborator

Yes, but there it's a string literal, not a variable.

@cogk
Copy link
Contributor

cogk commented May 21, 2024

__("d", null, "Days (Field: Duration)");

This translation context with Field: is specific to get_formatted_duration and the Duration control. In the select it would be more compatible with the rest of the framework to use case 2. suggested by Raffael (= the DocType name only)

@FHenry
Copy link
Contributor Author

FHenry commented May 21, 2024

ok, ok, I tried.

@barredterra, once again for me: RTFM , shame
I missed the bench get-untranslated and bench update-translations,
if that what you mean by "code generating the translation files"

But I will push a little more : The real "unique key" of translation is DocType (or context) + FieldName.
Imagine if in DocType Letter Head, there are two strings (label/output/select value/link....), one "Left" (as direction) and another another "Left" (as Exit), we were at the same point as today. In this case, you can always tell me that Frappe team will make the effort to use a synonymy in one of them, it will close the subject...., may be but may be not.
I will dig the path bench get-untranslated allow variable in context.

If you both think this PR have no chance to be merge, I will close it in favor of @cogk PR (#26425) (hoping it will be merge), but I still believe that all output have to be contextualize in a unique way.
The work you've done @barredterra on #24903 is amazing that's really reassuring to see how you push feature in Frappe Framework.

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.

None yet

3 participants