OEL-1160: Header refactor. #71

escuriola · 2022-02-16T17:31:36Z

https://citnet.tech.ec.europa.eu/CITnet/jira/browse/OEL-1160

oe_bootstrap_theme changes are requested to manage BCL EU/EC logos

drishu

Somethings not right in mobile view

modules/oe_whitelabel_helper/src/EuropeanUnionLanguages.php

modules/oe_whitelabel_helper/src/TwigExtension/TwigExtension.php

oe_whitelabel.theme

tests/src/Kernel/SiteBrandingBlockTest.php

drishu

Don't forget to add the new tag for the new logo location in OEBT

oe_whitelabel.theme

templates/overrides/navigation/block--system-branding-block.html.twig

modules/oe_whitelabel_helper/src/Plugin/Block/CorporateEcLogoBlock.php

templates/overrides/navigation/block--system-branding-block.html.twig

tests/src/Functional/CorporateHeaderLogosTest.php

tests/src/Kernel/SiteBrandingBlockTest.php

drishu · 2022-02-23T16:59:21Z

templates/overrides/navigation/block--system-branding-block.html.twig

+            {% set name_classes = [
+              'text-decoration-none',
+              'align-bottom',
+              light ? 'text-dark': 'text-white',


something like this needs to be added also to the title printed in mobile, see

brummbar · 2022-03-02T17:31:52Z

oe_whitelabel.theme

+ */
+function oe_whitelabel_preprocess_page(&$variables) {
+  if ($variables['bcl_component_library'] === 'ec') {
+    $variables['logo_aria_label'] = t('Home') . ' - ' . t('European Commission');


Suggested change

$variables['logo_aria_label'] = t('Home') . ' - ' . t('European Commission');

$variables['logo_aria_label'] = t('Home - European Commission');

brummbar · 2022-03-02T17:32:19Z

oe_whitelabel.theme

+    $site_logo_href = 'https://ec.europa.eu/info';
+  }
+  elseif ($variables['bcl_component_library'] === 'eu') {
+    $variables['logo_aria_label'] = t('Home') . ' - ' . t('European Union');


Suggested change

$variables['logo_aria_label'] = t('Home') . ' - ' . t('European Union');

$variables['logo_aria_label'] = t('Home - European Union');

brummbar · 2022-03-02T17:37:49Z

templates/overrides/navigation/block--system-branding-block.html.twig

+            {% set img_classes = ['d-none','d-lg-inline-block', 'text-decoration-none', 'site-logo'] %}
+            <a {{ create_attribute().addClass(img_classes).setAttribute('rel', 'Home') }} href="{{ path('<front>') }}">


Suggested change

{% set img_classes = ['d-none','d-lg-inline-block', 'text-decoration-none', 'site-logo'] %}

<a {{ create_attribute().addClass(img_classes).setAttribute('rel', 'Home') }} href="{{ path('<front>') }}">

<a class="d-none d-lg ...." href="{{ path('<front>') }}">

Rel attribute I didn't find a value for home https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/rel

brummbar · 2022-03-02T17:40:13Z

templates/overrides/navigation/region--navbar-right.html.twig

+#}
+{% set attributes = attributes.addClass('nav') %}
+{% if content %}
+  <ul{{ attributes }}>


Wait, region content inside a ul tag? This is weird.

brummbar · 2022-03-02T17:41:28Z

templates/overrides/page/page.html.twig

+    {% set attributes = create_attribute().addClass(['bcl-header', 'bcl-header--' ~ bcl_component_library]) %}
+    <header {{ attributes }}>


Just output in the header tag.

brummbar · 2022-03-02T17:57:21Z

templates/overrides/page/page.html.twig

-    {% else %}
-    <div class="container mt-5">
-    {% endif %}
+    <div class="container {{ page.header ? 'mt-2': 'mt-5' }} ">


Suggested change

<div class="container {{ page.header ? 'mt-2': 'mt-5' }} ">

<div class="container {{ page.header ? 'mt-2': 'mt-5' }}">

brummbar · 2022-03-02T17:58:13Z

tests/src/Kernel/SiteBrandingBlockTest.php

+    $builder = \Drupal::entityTypeManager()->getViewBuilder('block');
+    $build = $builder->view($entity, 'block');
+    $render = $this->container->get('renderer')->renderRoot($build);
+    $crawler = new Crawler($render->__toString());


Suggested change

$crawler = new Crawler($render->__toString());

$crawler = new Crawler((string) $render);

brummbar · 2022-03-02T20:06:54Z

templates/overrides/navigation/region--navbar-right.html.twig

+{#
+/**
+ * @file
+ * Default theme implementation to display a region.


Please update this. Write which template we are overriding too.

brummbar · 2022-03-02T20:32:42Z

templates/overrides/navigation/block--system-branding-block.html.twig

+      {% if light is not empty %}
+        {% set project_classes = project_classes ~ ' light' %}
+      {% endif %}
+      <div {{ create_attribute().addClass(project_classes) }}>


Suggested change

<div {{ create_attribute().addClass(project_classes) }}>

<div class="bcl-header__project{{ light is not empty ? ' light' }}">

brummbar · 2022-03-02T20:33:36Z

templates/overrides/navigation/block--system-branding-block.html.twig

+            </a>
+          {% endif %}
+          {% if site_name is not empty %}
+            {% set name_classes = [
+              'text-decoration-none',
+              'align-bottom',
+              'bcl-header__site-name',
+              'mw-100',
+            ] %}
+            {% set _site_name %}
+              <a {{ create_attribute().addClass(name_classes).setAttribute('rel', 'Home') }}
+                href="{{ path('<front>') }}">
+                {{ site_name }}
+              </a>
+            {% endset %}
+            {% set _site_name_classes = ['bcl-header__site-name', 'site-name'] %}
+            {% if bcl_component_library == 'neutral' %}
+              {% set _site_name_classes = _site_name_classes|merge(['h5', 'd-inline-block', 'd-lg-none']) %}
+            {% endif %}
+            <div {{ create_attribute().addClass(_site_name_classes) }}>
+              {{ _site_name }}
+            </div>
+          {% endif %}
+        </div>


Don't abuse attributes, if you can just output in html.

brummbar

Looks good, let's rebase and merge.

…ing-block.

…ate.

…tly to the html.

brummbar

Looks good now.

drishu

Lets add the diff to 1.x in composer

escuriola force-pushed the OEL-1160 branch 2 times, most recently from 123e223 to e5ec342 Compare February 21, 2022 14:48

drishu requested changes Feb 22, 2022

View reviewed changes

escuriola force-pushed the OEL-1160 branch from b95e939 to 4e4fb05 Compare February 22, 2022 08:11

drishu requested changes Feb 23, 2022

View reviewed changes

escuriola force-pushed the OEL-1160 branch from 9d93a38 to 28ba257 Compare February 23, 2022 16:09

drishu previously approved these changes Feb 23, 2022

View reviewed changes

escuriola dismissed drishu’s stale review via 025cc2c February 23, 2022 16:13

escuriola force-pushed the OEL-1160 branch from 28ba257 to 025cc2c Compare February 23, 2022 16:13

drishu requested changes Feb 23, 2022

View reviewed changes

escuriola force-pushed the OEL-1160 branch 2 times, most recently from 7748bf0 to 129eec2 Compare February 24, 2022 08:32

drishu previously approved these changes Feb 24, 2022

View reviewed changes

brummbar requested changes Mar 2, 2022

View reviewed changes

escuriola dismissed drishu’s stale review via 8cef142 March 3, 2022 12:19

escuriola force-pushed the OEL-1160 branch from 129eec2 to 8cef142 Compare March 3, 2022 12:19

brummbar previously approved these changes Mar 9, 2022

View reviewed changes

escuriola added 13 commits March 10, 2022 14:44

OEL-1160: Remove header pattern.

eba124f

OEL-1160: Remove extra CorporateBlock for logos, now are inside brand…

1ebf886

…ing-block.

OEL-1160: Add EuropeanLanguages helper class.

74df5df

OEL-1160: Create page template to BCL branding.

4671872

OEL-1160: Add block and remove header pattern from page template.

4da077e

OEL-1160: Add template to region branding header top right.

35ac939

OEL-1160: Add language filter to twig.

a96ec43

OEL-1160: Add page branding suggestion and preprocess variables.

b1225dc

OEL-1160: Adapt branding logos to component_library.

49ed6ca

OEL-1160: Fix theme logo and language management.

de13e0e

OEL-1160: Fixing test, WIP.

07613e3

OEL-1160: Remove branding page template. Put all header in page templ…

3aa5896

…ate.

OEL-1160: Branding with site name and site logo. Testing.

c3214dc

escuriola added 15 commits March 10, 2022 14:44

OEL-1160: Add main navigation block to header.

5fe9e52

OEL-1160: Remove logos from oe_whitelabel.

af0192c

OEL-1160: Remove obsolete blocks.

4d8c071

OEL-1160: Apply spaceless and minor block template refactor.

cc7da31

OEL-1160: Improve testing.

ff45cd9

OEL-1160: Minor page template refactor.

4196fe0

OEL-1160: Minor refactor, removing non-valid lines and perform comments.

35a6a4a

OEL-1160: Add class to link to display correctly on mobile.

657924a

OEL-1160: Refactor. Remove attributes object and add attributes direc…

b02a00e

…tly to the html.

OEL-1160: Refactor. Join home title.

7c587c9

OEL-1160: Refactor. Improve testing.

f345460

OEL-1160: Refactor. Fix template file information.

f7e58cc

OEL-1160: Refactor. Remove attributes object.

1510a63

OEL-1160: Refactor. Remove ul from header top right links.

cb67343

OEL-1160: Fix header branding top right links. Make them responsive.

4e7baef

escuriola dismissed brummbar’s stale review via 4e7baef March 10, 2022 13:44

escuriola force-pushed the OEL-1160 branch from e7e76c0 to 4e7baef Compare March 10, 2022 13:44

OEL-1160: Point to ticket branch for oe_bootstrap_theme.

2f5584a

brummbar previously approved these changes Mar 11, 2022

View reviewed changes

brummbar dismissed their stale review via 2f5584a March 11, 2022 20:12

brummbar force-pushed the OEL-1160 branch from 5c8fd92 to 2f5584a Compare March 11, 2022 20:12

OEL-1160: Remove patch.

24e2ad5

drishu approved these changes Mar 14, 2022

View reviewed changes

drishu requested changes Mar 14, 2022

View reviewed changes

OEL-1160: Add diff patch with 1.x.

315e126

drishu approved these changes Mar 14, 2022

View reviewed changes

brummbar approved these changes Mar 14, 2022

View reviewed changes

brummbar merged commit 50efe5b into 1.x Mar 14, 2022

brummbar deleted the OEL-1160 branch March 14, 2022 10:54

kp77 mentioned this pull request Apr 29, 2022

EU logo is not displayed on mobile #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEL-1160: Header refactor. #71

OEL-1160: Header refactor. #71

escuriola commented Feb 16, 2022 •

edited

Loading

drishu left a comment

drishu left a comment

drishu Feb 23, 2022 •

edited

Loading

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar Mar 2, 2022

brummbar left a comment

brummbar left a comment

drishu left a comment

	$variables['logo_aria_label'] = t('Home') . ' - ' . t('European Commission');
	$variables['logo_aria_label'] = t('Home - European Commission');

		{% set img_classes = ['d-none','d-lg-inline-block', 'text-decoration-none', 'site-logo'] %}
		<a {{ create_attribute().addClass(img_classes).setAttribute('rel', 'Home') }} href="{{ path('<front>') }}">

	{% set img_classes = ['d-none','d-lg-inline-block', 'text-decoration-none', 'site-logo'] %}
	<a {{ create_attribute().addClass(img_classes).setAttribute('rel', 'Home') }} href="{{ path('<front>') }}">
	<a class="d-none d-lg ...." href="{{ path('<front>') }}">

		{% set attributes = create_attribute().addClass(['bcl-header', 'bcl-header--' ~ bcl_component_library]) %}
		<header {{ attributes }}>

	<div class="container {{ page.header ? 'mt-2': 'mt-5' }} ">
	<div class="container {{ page.header ? 'mt-2': 'mt-5' }}">

	$crawler = new Crawler($render->__toString());
	$crawler = new Crawler((string) $render);

	<div {{ create_attribute().addClass(project_classes) }}>
	<div class="bcl-header__project{{ light is not empty ? ' light' }}">

OEL-1160: Header refactor. #71

OEL-1160: Header refactor. #71

Conversation

escuriola commented Feb 16, 2022 • edited Loading

drishu left a comment

Choose a reason for hiding this comment

drishu left a comment

Choose a reason for hiding this comment

drishu Feb 23, 2022 • edited Loading

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

brummbar left a comment

Choose a reason for hiding this comment

brummbar left a comment

Choose a reason for hiding this comment

drishu left a comment

Choose a reason for hiding this comment

escuriola commented Feb 16, 2022 •

edited

Loading

drishu Feb 23, 2022 •

edited

Loading