Code development platform for open source projects from the European Union institutions

Skip to content
Snippets Groups Projects

OEL-1167: Theme the content language switcher

Merged Francesco SARDARA requested to merge OEL-1167 into 1.x

Created by: GilNovacomm

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 6a0fbd27
  • 105 $plugin_block = $block_manager->createInstance('oe_multilingual_content_language_switcher', $config);
    106 $render = $plugin_block->build();
    107 $html = (string) $this->container->get('renderer')->renderRoot($render);
    108 $crawler = new Crawler($html);
    109
    110 // Make sure that content language switcher block is present.
    111 $actual = $crawler->filter('#dropdown-languages');
    112 $this->assertCount(1, $actual);
    113
    114 // Warning message doesn't contain the unavailable language, the translation
    115 // will have it.
    116 $this->assertUnavailableLanguage($crawler, 'This page is not available in български.');
    117
    118 // Make sure that available languages are properly rendered.
    119 $this->assertTranslationLinks($crawler, ['español', 'English']);
    120 }
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 6a0fbd27
  • 14 */
    15 class ContentLanguageSwitcherTest extends KernelTestBase {
    16 /**
    17 * {@inheritdoc}
    18 */
    19 public static $modules = [
    20 'block',
    21 'content_translation',
    22 'language',
    23 'locale',
    24 'node',
    25 'oe_bootstrap_theme_helper',
    26 'oe_multilingual',
    27 'oe_whitelabel_helper',
    28 'oe_whitelabel_multilingual',
    29 'system',
    • Created by: drishu

      can you double check this test is running in drone, like make it fail and see it runs. Asking because like this it was failing in my local, had to add 'oe_corporate_blocks'

    • Created by: GilNovacomm

      It did pass on drone, but locally after rebasing it I do need the same dependency.

  • Francesco SARDARA
  • Created by: drishu

    Review: Changes requested

    • also place the block so it appears above the content;
    • replace the enabling of oe_multilingual in the runner with this module;
    • and update the branch of course
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit af8b27d0
  • 5 namespace Drupal\Tests\oe_whitelabel\Kernel;
    6
    7 use Drupal\node\Entity\Node;
    8 use Drupal\KernelTests\KernelTestBase;
    9 use Symfony\Component\DomCrawler\Crawler;
    10 use Symfony\Component\HttpFoundation\Request;
    11
    12 /**
    13 * Test content language switcher rendering.
    14 */
    15 class ContentLanguageSwitcherTest extends KernelTestBase {
    16 /**
    17 * {@inheritdoc}
    18 */
    19 public static $modules = [
    20 'block',
  • Created by: drishu

    Review: Changes requested

    Again:

    • also place the block so it appears above the content (to make sure of this also place the content block, see oe_theme);
    • replace the enabling of oe_multilingual in the runner with this module;
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 604d47d0
  • 130 $html = (string) $this->container->get('renderer')->renderRoot($render);
    131 $crawler = new Crawler($html);
    132
    133 // Verify that the requested language is set as unavailable.
    134 $this->assertUnavailableLanguage($crawler, 'This page is not available in español.');
    135
    136 // Verify that the content has been rendered in the fallback language.
    137 $this->assertSelectedLanguage($crawler, 'English');
    138
    139 // Re-render the block assuming a request to the base language English
    140 // version of the node.
    141 $this->setCurrentRequest('/en/node/' . $node->id());
    142
    143 $html = (string) $this->container->get('renderer')->renderRoot($render);
    144 $crawler = new Crawler($html);
    145 $render = $plugin_block->build();
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 49a11c4a
  • 129
    130 $html = (string) $this->container->get('renderer')->renderRoot($render);
    131 $crawler = new Crawler($html);
    132
    133 // Verify that the requested language is set as unavailable.
    134 $this->assertUnavailableLanguage($crawler, 'This page is not available in español.');
    135
    136 // Verify that the content has been rendered in the fallback language.
    137 $this->assertSelectedLanguage($crawler, 'English');
    138
    139 // Simulate a request to the canonical route of the node base language.
    140 $this->setCurrentRequest('/node/' . $node->id());
    141
    142 $html = (string) $this->container->get('renderer')->renderRoot($render);
    143 $crawler = new Crawler($html);
    144
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 8 {% set _message = {
    9 message: "This page is not available in @language."|t({'@language': unavailable_language}),
    10 path: bcl_icon_path,
    11 variant: "warning",
    12 }%}
    13 {% set _expandable = {
    14 label: "Choose another language"|t,
    15 icon: {
    16 name: "caret-down-fill",
    17 path: bcl_icon_path,
    18 },
    19 outline: "true",
    20 } %}
    21
    22 {% set _languages = languages|default([]) %}
    23 {% set _expandable_attributes = _expandable.attributes ?: create_attribute() %}
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 5 @todo Call include once BCL 0.20 is available.
    6 #}
    7 {% set _id = "dropdown-languages" %}
    8 {% set _message = {
    9 message: "This page is not available in @language."|t({'@language': unavailable_language}),
    10 path: bcl_icon_path,
    11 variant: "warning",
    12 }%}
    13 {% set _expandable = {
    14 label: "Choose another language"|t,
    15 icon: {
    16 name: "caret-down-fill",
    17 path: bcl_icon_path,
    18 },
    19 outline: "true",
    20 } %}
    • Created by: donquixote

      Can we name this _expand_button instead of _expandable? Or, if up to me, we could use inline value instead of a variable - see below.

    • Created by: donquixote

      Ok, I see now that lots in here is copied from bcl-language-switcher.html.twig. If the goal is to be as close as possible to the bcl template, then some of my comments here can be ignored.

    • Created by: drishu

      yes, we don't have BCL 0.20 yet, but as soon as we do it will call a template from there, so no need to QA on it

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 21
    22 {% set _languages = languages|default([]) %}
    23 {% set _expandable_attributes = _expandable.attributes ?: create_attribute() %}
    24 {% set _expandable_attributes = _expandable_attributes
    25 .setAttribute('data-bs-toggle', 'collapse')
    26 .setAttribute('autocomplete', 'off')
    27 .setAttribute('aria-expanded', 'false')
    28 .setAttribute('aria-controls', _id)
    29 .setAttribute('data-bs-target', '#' ~ _id)
    30 %}
    31
    32 {% include '@oe-bcl/bcl-alert/alert.html.twig' with _message only %}
    33 <div class="mb-3">
    34 {% include '@oe-bcl/bcl-button/button.html.twig' with _expandable|merge({
    35 attributes: _expandable_attributes
    36 }) only %}
    • Created by: donquixote

      Can we we drop the variable and use an inline value? (could be just my personal preference) E.g.

          {# Dropdown-like button to hide/reveal the language list. #}
          {% include '@oe-bcl/bcl-button/button.html.twig' with {
            label: "Choose another language"|t,
            icon: {
              name: "caret-down-fill",
              path: bcl_icon_path,
            },
            outline: "true",
            attributes: create_attribute()
              .setAttribute('data-bs-toggle', 'collapse')
              .setAttribute('autocomplete', 'off')
              .setAttribute('aria-expanded', 'false')
              .setAttribute('aria-controls', _id)
              .setAttribute('data-bs-target', '#' ~ _id),
          } only %}
    • Created by: drishu

      no, let's no QA on it

  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 27 .setAttribute('aria-expanded', 'false')
    28 .setAttribute('aria-controls', _id)
    29 .setAttribute('data-bs-target', '#' ~ _id)
    30 %}
    31
    32 {% include '@oe-bcl/bcl-alert/alert.html.twig' with _message only %}
    33 <div class="mb-3">
    34 {% include '@oe-bcl/bcl-button/button.html.twig' with _expandable|merge({
    35 attributes: _expandable_attributes
    36 }) only %}
    37
    38 <div class="collapse mt-3" id="{{ _id }}">
    39 <div
    40 class="d-md-grid"
    41 style="grid-auto-flow: column; grid-template-rows: repeat(4, 1fr)"
    42 >
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 7 {% set _id = "dropdown-languages" %}
    8 {% set _message = {
    9 message: "This page is not available in @language."|t({'@language': unavailable_language}),
    10 path: bcl_icon_path,
    11 variant: "warning",
    12 }%}
    13 {% set _expandable = {
    14 label: "Choose another language"|t,
    15 icon: {
    16 name: "caret-down-fill",
    17 path: bcl_icon_path,
    18 },
    19 outline: "true",
    20 } %}
    21
    22 {% set _languages = languages|default([]) %}
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 17 path: bcl_icon_path,
    18 },
    19 outline: "true",
    20 } %}
    21
    22 {% set _languages = languages|default([]) %}
    23 {% set _expandable_attributes = _expandable.attributes ?: create_attribute() %}
    24 {% set _expandable_attributes = _expandable_attributes
    25 .setAttribute('data-bs-toggle', 'collapse')
    26 .setAttribute('autocomplete', 'off')
    27 .setAttribute('aria-expanded', 'false')
    28 .setAttribute('aria-controls', _id)
    29 .setAttribute('data-bs-target', '#' ~ _id)
    30 %}
    31
    32 {% include '@oe-bcl/bcl-alert/alert.html.twig' with _message only %}
    • Created by: donquixote

      Again I would prefer inline parameters instead of a variable. But I am ok if the goal is to be aligned with the bcl template (snapshot at 67447742)

      E.g.

      {% include '@oe-bcl/bcl-alert/alert.html.twig' with {
        message: "This page is not available in @language."|t({'@language': unavailable_language}),
        path: bcl_icon_path,
        variant: "warning",
      } only %}
    • Created by: drishu

      its BCL, will be replaced

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 40 /** @var \Drupal\Core\Entity\EntityInterface $translation */
    41 $translation = $multilingual_helper->getCurrentLanguageEntityTranslation($entity);
    42 $variables['languages'][] = [
    43 'path' => $translation->toUrl()->setAbsolute(TRUE)->toString(),
    44 'hreflang' => $translation->language()->getId(),
    45 'label' => $languages[$translation->language()->getId()]->getName(),
    46 'current' => TRUE,
    47 'icon_position' => 'before',
    48 'remove_icon_spacers' => FALSE,
    49 'icon' => [
    50 'name' => 'check-lg',
    51 'path' => $variables['bcl_icon_path'],
    52 'size' => 'xs',
    53 '#attributes' => ['class' => ['me-2']],
    54 ],
    55 ];
    • Created by: donquixote

      I wonder, why does ContentLanguageBlock from oe_multilingual not provide this link?

      Also, when we show this link, as a visitor I think I would want to see something like: "✓ English (shown instead of Polish)" instead of just "✓ English", if Polish is the language where we don't have a translation.

      But it seems this has been decided in BCL and we are not going to change it here.

    • Created by: drishu

      I think that just indicates the fallback language

    • Created by: donquixote

      I see that, but it is not immediately obvious for a visitor.

    • Created by: donquixote

      Anyway I am resolving this.

  • Created by: donquixote

    I have some general feedback about UX for the design in BCL. This is not really the place to fix this, but I am going to mention it anyway:

    • The open/close button looks like a radio element, but it does not behave as such, defying user expectations.
    • The open/close icon always points down. It should point up when the list is opened.

    Not a blocker for this PR.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 43 38 - block.block.oe_whitelabel_branding
    39 - block.block.oe_whitelabel_breadcrumbs
    40 - block.block.oe_whitelabel_ec_corporate_footer
    41 - block.block.oe_whitelabel_eu_corporate_footer
    42 - block.block.oe_whitelabel_eulogin
    43 - block.block.oe_whitelabel_language_switcher
    44 44 - block.block.oe_whitelabel_local_actions
    45 45 - block.block.oe_whitelabel_local_tasks
    46 - block.block.oe_whitelabel_main_navigation
    47 - block.block.oe_whitelabel_main_page_content
    46 48 - block.block.oe_whitelabel_messages
    47 - block.block.oe_whitelabel_page_title
    48 - block.block.whitelabelsearchblock
    49 - cas.settings.yml
    49 - block.block.oe_whitelabel_neutral_footer
    50 - block.block.oe_whitelabel_search_form
    • Created by: donquixote

      I found that these entries are already wrong in 1.x. Also in two other files:

      • oe_whitelabel_starter_event.info.yml (wrong order)
      • oe_whitelabel_starter_news.info.yml (".yml" suffix)

      Imo we should have a separate branch to fix all of the entries, and then we rebase. But I can accept if we want to do this here to move forward.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 3 3 dependencies:
    4 4 module:
    5 5 - oe_corporate_blocks
    6 - oe_whitelabel_helper
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 46 'current' => TRUE,
    47 'icon_position' => 'before',
    48 'remove_icon_spacers' => FALSE,
    49 'icon' => [
    50 'name' => 'check-lg',
    51 'path' => $variables['bcl_icon_path'],
    52 'size' => 'xs',
    53 '#attributes' => ['class' => ['me-2']],
    54 ],
    55 ];
    56 }
    57
    58 /**
    59 * Implements hook_preprocess_links__language_block().
    60 */
    61 function oe_whitelabel_multilingual_preprocess_links__language_block(&$variables) {
    • Created by: donquixote

      TBD: We could follow the pattern where we implement the preprocess hook on behalf of oe_whitelabel theme, instead of the module. This would prevent the hook from running if oe_whitelabel is not the active theme, e.g. during a test when we only test the structure and not the theme. I think it won't matter in this case, but we want to establish best practices that are safe to use elsewhere.

    • Created by: donquixote

      (Arguments could be made that all the preprocess functions should be in the theme, not in a submodule, to prevent that the template is ever used without the preprocess. Still, both would depend on the exact config coming from a module.)

    • Created by: drishu

      Let's keep it like this for now, and prep a discussion among leads to establish final word on this @donquixote

  • Francesco SARDARA
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 72 // @see CurrentUserContext::getRuntimeContexts().
    73 // @see EntityConverter::convert().
    74 module_load_include('install', 'user');
    75 user_install();
    76
    77 \Drupal::service('kernel')->rebuildContainer();
    78 }
    79
    80 /**
    81 * Tests the rendering of the language switcher block.
    82 */
    83 public function testMultilingualLanguageSwitcherBlockRendering(): void {
    84 $node = Node::create([
    85 'title' => 'Hello, world!',
    86 'type' => 'oe_demo_translatable_page',
    87 ]);
    • Created by: donquixote

      This is very interesting. The oe_demo_translatable_page content type is defined in oe_multilingual_demo, but that module is not enabled in this test. But the test still passes. In fact it still passes if we insert any arbitrary string for the node type.

    • Created by: drishu

      indeed, I noticed that myself, but I kept it like that so its as close as possible to the test in oe_theme

    • Created by: donquixote

      Let's add an inline comment:

          $node = Node::create([
            'title' => 'Hello, world!',
            // The node type does not actually exist in this test, but it works.
            'type' => 'oe_demo_translatable_page',
          ]);
  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit 5eaf632d
  • 87 ]);
    88 /** @var \Drupal\Core\Entity\EntityInterface $translation */
    89 $node->addTranslation('es', ['title' => '¡Hola mundo!'])->save();
    90
    91 // Simulate a request to the canonical route of the node with Bulgarian
    92 // language prefix.
    93 $this->setCurrentRequest('/bg/node/' . $node->id());
    94
    95 // Setup and render language switcher block.
    96 $block_manager = \Drupal::service('plugin.manager.block');
    97 $config = [
    98 'id' => 'oe_multilingual_content_language_switcher',
    99 'label' => 'Content language switcher',
    100 'provider' => 'oe_multilingual',
    101 'label_display' => '0',
    102 ];
  • Created by: donquixote

    Review: Commented

    I left some comments. Maybe some of these should lead to changes, but some maybe can be ignored, if there are good reasons why we do it this way, or decisions were already made in the past.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit e5b2f37e
  • 109 109 $crawler = new Crawler($html);
    110 110
    111 111 // Make sure that content language switcher block is present.
    112 $actual = $crawler->filter('#dropdown-languages');
    112 $actual = $crawler->filter('div.collapse.mt-3');
    • Created by: donquixote

      I don't like to see the .mt-3 here, but I suppose we have to for disambiguation? Worst case we have to update this test when this class changes.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit e5b2f37e
  • 57 59 ],
    58 60 ];
    59 61 }
    62
    63 // Generate required ids.
    64 $variables['expandable_id'] = Html::getUniqueId('bcl-expandable');
    • Created by: donquixote

      I wonder if caching (render cache) could cause the same id to be used elsewhere on the same page. But I think this would need a very convoluted scenario, which we should ignore for the time being. So I am ok with it.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit bc274a47
  • 44 if ($translation->language()->getId() !== 'und') {
    45 $variables['languages'][] = [
    46 'path' => $translation->toUrl()->setAbsolute(TRUE)->toString(),
    47 'hreflang' => $translation->language()->getId(),
    48 'label' => $languages[$translation->language()->getId()]->getName(),
    49 'current' => TRUE,
    50 'icon_position' => 'before',
    51 'remove_icon_spacers' => FALSE,
    52 'icon' => [
    53 'name' => 'check-lg',
    54 'path' => $variables['bcl_icon_path'],
    55 'size' => 'xs',
    56 '#attributes' => ['class' => ['me-2']],
    57 ],
    58 ];
    59 }
    • Created by: donquixote

      What is the scenario where this happens?

    • Created by: donquixote

      Btw I was considering to suggest early return pattern here (invert the if to get rid of indentation). But then we would have to drop the early return when we add the $variables['expandable_id']. This is why I prefer to use early return only in functions with a single responsibility - which is often not the case for preprocess. So ok to keep it this way.

    • Created by: donquixote

      Also if language is 'und', will the message "This page is not available in ***." make sense for all cases?

      I think we should settle for something that works, we have no time to investigate all possible scenarios. If you say you want to keep the current version, let's go with it.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit bc274a47
  • 47 'icon_position' => 'before',
    48 'remove_icon_spacers' => FALSE,
    49 'icon' => [
    50 'name' => 'check-lg',
    51 'path' => $variables['bcl_icon_path'],
    52 'size' => 'xs',
    53 '#attributes' => ['class' => ['me-2']],
    54 ],
    55 ];
    42 // If we don't have a language id defined yet, the current translation wasn't
    43 // saved, so we don't add it to the list.
    44 if ($translation->language()->getId() !== 'und') {
    45 $variables['languages'][] = [
    46 'path' => $translation->toUrl()->setAbsolute(TRUE)->toString(),
    47 'hreflang' => $translation->language()->getId(),
    48 'label' => $languages[$translation->language()->getId()]->getName(),
    • Created by: donquixote

      We could introduce a local var for $translation->language()->getId(), to avoid the repeated call, e.g. $current_content_language_id.

      We could also check whether $languages[$current_content_language_id] exists. Would be strange if it does not. Perhaps if a site language is deleted, renamed or disabled, while translations still exist? I don't think it will happen.

  • Francesco SARDARA
    Francesco SARDARA @brummbar started a thread on commit bc274a47
  • 43 'path' => $translation->toUrl()->setAbsolute(TRUE)->toString(),
    44 'hreflang' => $translation->language()->getId(),
    45 'label' => $languages[$translation->language()->getId()]->getName(),
    46 'current' => TRUE,
    47 'icon_position' => 'before',
    48 'remove_icon_spacers' => FALSE,
    49 'icon' => [
    50 'name' => 'check-lg',
    51 'path' => $variables['bcl_icon_path'],
    52 'size' => 'xs',
    53 '#attributes' => ['class' => ['me-2']],
    54 ],
    55 ];
    42 // If we don't have a language id defined yet, the current translation wasn't
    43 // saved, so we don't add it to the list.
    44 if ($translation->language()->getId() !== 'und') {
  • Created by: drishu

    Review: Approved

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading