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

do not translate language options on post box #155

Merged
merged 7 commits into from
Nov 4, 2023

Conversation

e-five256
Copy link
Member

@e-five256 e-five256 commented Oct 28, 2023

pages that loaded the reply/comment/post box had 634 translation misses (and likely each time it was added to the page). this was because it attempted to translate the language labels, but languages should not be translated but spelled using their own language

I'm still not quite sure I fixed this properly, as I'm not sure what the ternary if was trying to accomplish

@e-five256 e-five256 added bug Something isn't working low priority does not impact production code labels Oct 28, 2023
@@ -2,12 +2,12 @@
{%- block choice_widget_options -%}
{% for group_label, choice in options %}
{%- if choice is iterable -%}
<optgroup label="{{ choice_translation_domain is same as(false) ? group_label : group_label|trans({}, choice_translation_domain) }}">
Copy link
Member Author

@e-five256 e-five256 Oct 29, 2023

Choose a reason for hiding this comment

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

I'm not quite understanding what choice_translation_domain is meant to represent in this file. I don't think it's being used from the controller, unless I'm looking in the wrong place which is possible. Maybe it was meant to be choice.translation_domain, on the iterator? As there are instances of translation_domain

edit: doesn't seem to exist on this particular iterator though so I guess that's not it, will have to investigate more

Copy link
Member Author

Choose a reason for hiding this comment

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

@e-five256 e-five256 marked this pull request as ready for review November 2, 2023 03:06
@e-five256
Copy link
Member Author

e-five256 commented Nov 2, 2023

After a bunch of investigation, I think I understand what happened now.

First, choice_translation_domain always appears null on pages I can find this lang select form on (single entry, inline reply, profile, microblog). As null does not match false, it goes into the translate filter with the default language as described in the docs I linked above.

After trying to figure this out, I stumbled on the default twig form select layout, which this almost matches, which made me realize the comment at the top of this template is saying just that, it's a near copy of the twig select form.

So now I wasn't quite sure the best way to handle this: Removing this code strays further from the default template, but we should know the languages will never translate, which is all this component is for, so I'm not sure if that causes issues aside from making it more custom.

An alternative might be to see if the default twig component has fixed any issues to preferred choice this template mentions.

(On the same note, choice is never iterable so we could remove that check as well if we wanted, there were no instances I saw of the lang select having optgroup blocks. But that seemed less needed.)

Copy link
Member

@nobodyatroot nobodyatroot left a comment

Choose a reason for hiding this comment

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

I think this has been open for comment long enough, I don't see any reason to hold it back.

@e-five256 e-five256 merged commit 11cda83 into main Nov 4, 2023
6 checks passed
@e-five256 e-five256 deleted the e5/lang-select-translations branch November 4, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority does not impact production code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants