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

IBX-8508: Fixed translation_domain working incorrectly for dropdowns #1286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Jul 2, 2024

🎫 Issue IBX-8508

For QA:

We will need to check label translations in other parts of admin UI. Some parts might no longer be translated, since they would be using old translation_domain option (instead of choice_translation_domain, as they should).

Description:

Relevant excerpt from discussion that was happening in a different, closed source PR:

The reason placeholder option is not run through translation is that we are overwriting choice_widget behavior in vendor/ibexa/admin-ui/src/bundle/Resources/views/themes/admin/ui/form_fields/dropdown_widget.html.twig.

What should normally happen in choice_widget_collapsed (which is the block that is usually responsible for rendering <select> element for ChoiceType) is that placeholder option should be run through translation (marked as diff so you can see the relevant part):

{%- block choice_widget_collapsed -%}
    {%- if required and placeholder is none and not placeholder_in_choices and not multiple and (attr.size is not defined or attr.size <= 1) -%}
        {% set required = false %}
    {%- endif -%}
    <select {{ block('widget_attributes') }}{% if multiple %} multiple="multiple"{% endif %}>
+        {%- if placeholder is not none -%}
+            <option value=""{% if required and value is empty %} selected="selected"{% endif %}>{{ placeholder != '' ? (translation_domain is same as(false) ? placeholder : placeholder|trans({}, translation_domain)) }}</option>
+        {%- endif -%}   
        {%- if preferred_choices|length > 0 -%}
            {% set options = preferred_choices %}
            {% set render_preferred_choices = true %}
            {{- block('choice_widget_options') -}}
            {%- if choices|length > 0 and separator is not none -%}
                <option disabled="disabled">{{ separator }}</option>
            {%- endif -%}
        {%- endif -%}
        {%- set options = choices -%}
        {%- set render_preferred_choices = false -%}
        {{- block('choice_widget_options') -}}
    </select>
{%- endblock choice_widget_collapsed -%}

However, it does not happen for our template. placeholder is still passed onto the subtemplate, but translation domain uses choice_translation_domain option. This effectively disables translation in this case.

{%- block choice_widget -%}
    {% if expanded %}
        {{- block('choice_widget_expanded') -}}
    {% else %}
        {% set attr = attr|default({})|merge({
            'class': (attr.class|default('') ~ ' ibexa-input ibexa-input--select')|trim,
        }) %}
        {% set source %}
            {{- block('choice_widget_collapsed') -}}
        {% endset %}

        {% include '@ibexadesign/ui/component/dropdown/dropdown.html.twig' with {
            source: source,
            choices: choices,
            preferred_choices: preferred_choices,
            value: form.vars.value,
            multiple: form.vars.multiple,
            custom_init: attr.dropdown_custom_init|default(false),
            class: attr.dropdown_class|default(''),
+            translation_domain: choice_translation_domain|default(false),
            custom_form: false,
            is_disabled: attr.disabled|default(false) or disabled|default(false) or dropdown_disabled|default(false),
            is_hidden: attr.dropdown_hidden|default(false),
            is_small: attr.is_small|default(false),
            is_ghost: attr.is_ghost|default(false),
            is_selector: attr.is_selector|default(false),
            is_dynamic: attr.is_dynamic|default(false),
            has_select_all_toggler: attr.has_select_all_toggler|default(false),
+            placeholder: attr.placeholder|default(placeholder),
            selected_item_template_path: selected_item_template_path|default(null),
            list_item_template_path: list_item_template_path|default(null),
            items_class: items_class|default(null),
            items_list_header: items_list_header|default(null),
        } only %}
    {% endif %}
{%- endblock choice_widget -%}

In theory placeholder and all possible options should have different translation domains (placeholder should use translation_domain, choices should use choice_translation_domain - you don't want to translate options oftentimes), but we are not respecting this setup.

You can see it when adding {{ dump(placeholder, translation_domain) }} to vendor/ibexa/admin-ui/src/bundle/Resources/views/themes/admin/ui/component/dropdown/dropdown.html.twig:
image

The moment I set choice_translation_domain option it changes:
image
but note that because of this, all choices are run through translation (which they shouldn't!) because placeholder and choices share translation domain setting.

With this approach and properly set translation_domain and choice_translation_domain:

        $builder->add('country', CountryType::class, [
            'required' => $builder->getRequired(),
            'label' => /** @Desc("Country") */ 'ibexa.address.country',
            'placeholder' => /** @Desc("Name") */ 'ibexa.address.name',
            'translation_domain' => 'ibexa_fieldtype_address',
        ]);

image

Documentation:

@Steveb-p Steveb-p changed the base branch from main to 4.6 July 2, 2024 14:06
@Steveb-p Steveb-p requested a review from dew326 July 2, 2024 14:08
@mikadamczyk
Copy link
Contributor

@Steveb-p is there any Jira issue related to this PR?

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Jul 9, 2024

After internal sync we decided to push this change onto 5.0 series to prevent breaks.

@Steveb-p Steveb-p changed the base branch from 4.6 to main July 9, 2024 09:20
@Steveb-p Steveb-p force-pushed the fix-dropdown-translation-domain branch from 31a2674 to a46d436 Compare July 9, 2024 09:23
Copy link

sonarqubecloud bot commented Jul 9, 2024

@Steveb-p Steveb-p changed the title Fixed translation_domain working incorrectly for dropdowns IBX-8508: Fixed translation_domain working incorrectly for dropdowns Jul 9, 2024
@Steveb-p Steveb-p marked this pull request as ready for review July 9, 2024 09:27
@Steveb-p Steveb-p requested a review from a team July 9, 2024 09:37
@konradoboza konradoboza requested a review from a team July 9, 2024 09:40
@Steveb-p Steveb-p added the Bug Something isn't working label Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants