OEL-1167: Theme the content language switcher #94

GilNovacomm · 2022-03-14T09:00:52Z

No description provided.

drishu

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

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

...oe_whitelabel_multilingual/templates/links--oe-multilingual-content-language-block.html.twig

tests/src/Kernel/ContentLanguageSwitcherTest.php

drishu

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;

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

templates/overrides/navigation/links--oe-multilingual-content-language-block.html.twig

tests/src/Kernel/ContentLanguageSwitcherTest.php

...whitelabel_multilingual/config/optional/block.block.openeuropa_content_language_switcher.yml

...s/oe_whitelabel_multilingual/config/optional/block.block.oe_whitelabel_main_page_content.yml

...whitelabel_multilingual/config/optional/block.block.openeuropa_content_language_switcher.yml

…eparate block from bottom text.

…gual on block theming.

…base language.

…ender.

…cement.

donquixote · 2022-03-19T20:33:23Z

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.

donquixote

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.

templates/overrides/navigation/links--oe-multilingual-content-language-block.html.twig

donquixote · 2022-03-19T19:43:56Z

templates/overrides/navigation/links--oe-multilingual-content-language-block.html.twig

+  _id, _message and _expandable are overwritten.
+  @todo Call include once BCL 0.20 is available.
+  #}
+  {% set _id = "dropdown-languages" %}


Is this id unique enough?
We could use 'dropdown-content-language'.

We could also use a randomized id, but this is bad for caching I think.

templates/overrides/navigation/links--oe-multilingual-content-language-block.html.twig

config/optional/block.block.oe_whitelabel_ec_corporate_footer.yml

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

donquixote · 2022-03-19T21:42:59Z

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

+  // Add the current language to the list.
+  $multilingual_helper = \Drupal::service('oe_multilingual.helper');
+  $entity = $multilingual_helper->getEntityFromCurrentRoute();
+  /** @var \Drupal\Core\Entity\EntityInterface $translation */


Put a @var doc on the $multilingual_helper service above, then you don't need one here.

tests/src/Kernel/ContentLanguageSwitcherTest.php

donquixote · 2022-03-21T14:30:45Z

tests/src/Kernel/ContentLanguageSwitcherTest.php

@@ -109,7 +109,7 @@ public function testMultilingualLanguageSwitcherBlockRendering(): void {
    $crawler = new Crawler($html);

    // Make sure that content language switcher block is present.
-    $actual = $crawler->filter('#dropdown-languages');
+    $actual = $crawler->filter('div.collapse.mt-3');


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.

donquixote · 2022-03-21T14:32:48Z

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

@@ -57,6 +59,9 @@ function oe_whitelabel_multilingual_preprocess_links__oe_multilingual_content_la
      ],
    ];
  }
+
+  // Generate required ids.
+  $variables['expandable_id'] = Html::getUniqueId('bcl-expandable');


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.

donquixote · 2022-03-21T14:46:15Z

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

+        '#attributes' => ['class' => ['me-2']],
+      ],
+    ];
+  }


What is the scenario where this happens?

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.

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.

donquixote · 2022-03-21T14:53:01Z

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

+    $variables['languages'][] = [
+      'path' => $translation->toUrl()->setAbsolute(TRUE)->toString(),
+      'hreflang' => $translation->language()->getId(),
+      'label' => $languages[$translation->language()->getId()]->getName(),


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.

donquixote · 2022-03-21T15:16:36Z

modules/oe_whitelabel_multilingual/oe_whitelabel_multilingual.module

-  ];
+  // If we don't have a language id defined yet, the current translation wasn't
+  // saved, so we don't add it to the list.
+  if ($translation->language()->getId() !== 'und') {


I notice you are using 'und' string literal instead of a constant.
In Drupal 7 I would have told you to use LANGUAGE_NONE.
Now this has changed, see LANGUAGE_NONE changed to LANGUAGE_NOT_SPECIFIED, LANGUAGE_NOT_APPLICABLE and LANGUAGE_MULTIPLE added

Not sure if we also need to support other possible values.
Perhaps safer to just check isset($languages[$current_content_language_id])?

donquixote

Good enough, we might fine-tune in follow-up.

drishu requested changes Mar 15, 2022

View reviewed changes

GilNovacomm force-pushed the OEL-1167 branch 2 times, most recently from 5623683 to c6e37b8 Compare March 16, 2022 16:48

drishu requested changes Mar 16, 2022

View reviewed changes

drishu reviewed Mar 16, 2022

View reviewed changes

tests/src/Kernel/ContentLanguageSwitcherTest.php Outdated Show resolved Hide resolved

GilNovacomm force-pushed the OEL-1167 branch 3 times, most recently from 8d71975 to 49a11c4 Compare March 17, 2022 11:00

drishu reviewed Mar 17, 2022

View reviewed changes

tests/src/Kernel/ContentLanguageSwitcherTest.php Outdated Show resolved Hide resolved

drishu requested changes Mar 17, 2022

View reviewed changes

gilmarfdelima and others added 20 commits March 17, 2022 22:44

OEL-1167: Create oe_whitelabel_multilingual submodule.

8c6ef00

OEL-1167: Create the language switcher block template.

54fb4b1

OEL-1167: Preprocess variables for the language switcher template.

4633bca

OEL-1167: Refactor oe_whitelabel_multilingual.module.

3ed54e2

OEL-1167: Use placeholders on alert text and add a margin bottom to s…

ec1ab50

…eparate block from bottom text.

OEL-1167: Create whitelabel multilingual kernel test.

d2d4536

OEL-1167: Move template to avoid dependency on oe_whitelabel_multilin…

a369c61

…gual on block theming.

OEL-1167: Update tests.

af44289

OEL-1167: Fix library commentary, and link icon definition.

ab65900

OEL-1167: Remove unecessary registry empty statement.

a58da02

OEL-1167: Assert block is not shown in test when viewing node on its …

e812d79

…base language.

OEL-1167: Enable oe_whitelabel_multilingual during installation.

8e06e95

OEL-1167: Sort test module dependencies and remove duplicated block r…

7f61d64

…ender.

OEL-1167: Code standards.

52f27eb

OEL-1167: Update template comment and icon path.

862198b

OEL-1167: Place the main page content block to ensure block order pla…

6bdc691

…cement.

OEL-1167: Update config dependency.

b90af4f

OEL-1167: Place the language switcher block on the page content region.

d6111df

OEL-1167: Move blocks to root module config/optional folder.

d0341e0

OEL-1167: Remove config dependency.

44095f0

drishu added 2 commits March 18, 2022 12:29

OEL-1167: Export block config.

1b27d53

OEL-1167: Rename content switcher in config_devel.

7bf25c4

drishu previously approved these changes Mar 18, 2022

View reviewed changes

OEL-1167: Remove content switcher block export.

1586043

drishu dismissed their stale review via 1586043 March 18, 2022 12:59

drishu previously approved these changes Mar 18, 2022

View reviewed changes

OEL-1167: Remove language_block preprocess from root.

777b1ae

drishu dismissed their stale review via 777b1ae March 18, 2022 14:47

drishu previously approved these changes Mar 18, 2022

View reviewed changes

OEL-1167: Fix typo in links block preprocess.

de3b993

drishu dismissed their stale review via de3b993 March 18, 2022 15:40

drishu previously approved these changes Mar 18, 2022

View reviewed changes

OEL-1167: Add new module dependency to multilingual kernel test.

5eaf632

drishu dismissed their stale review via 5eaf632 March 18, 2022 15:52

donquixote reviewed Mar 19, 2022

View reviewed changes

drishu added 4 commits March 21, 2022 15:20

OEL-1167: Account for language und.

bc274a4

OEL-1167: Generate expandable id.

e5b2f37

OEL-1167: Fix config_devel settings.

34998d5

OEL-1167: Move var doc to multilingual helper service.

023a770

donquixote reviewed Mar 21, 2022

View reviewed changes

drishu approved these changes Mar 21, 2022

View reviewed changes

donquixote approved these changes Mar 21, 2022

View reviewed changes

drishu changed the title ~~OEL-1167 Multilingual submodule~~ OEL-1167: Theme the content language switcher Mar 21, 2022

drishu merged commit 1b1df0d into 1.x Mar 21, 2022

drishu deleted the OEL-1167 branch March 21, 2022 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEL-1167: Theme the content language switcher #94

OEL-1167: Theme the content language switcher #94

GilNovacomm commented Mar 14, 2022

drishu left a comment •

edited

Loading

drishu left a comment •

edited by GilNovacomm

Loading

donquixote commented Mar 19, 2022

donquixote left a comment

donquixote Mar 19, 2022

donquixote Mar 19, 2022

donquixote Mar 19, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote Mar 21, 2022

donquixote left a comment

OEL-1167: Theme the content language switcher #94

OEL-1167: Theme the content language switcher #94

Conversation

GilNovacomm commented Mar 14, 2022

drishu left a comment • edited Loading

Choose a reason for hiding this comment

drishu left a comment • edited by GilNovacomm Loading

Choose a reason for hiding this comment

donquixote commented Mar 19, 2022

donquixote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote left a comment

Choose a reason for hiding this comment

drishu left a comment •

edited

Loading

drishu left a comment •

edited by GilNovacomm

Loading