From 5a2b8f7ff9f54c18cc9fa0204400d9891dad9d13 Mon Sep 17 00:00:00 2001
From: Thomas Rupprecht <rupprecht.thomas@gmail.com>
Date: Sun, 22 Jan 2023 19:16:33 +0100
Subject: [PATCH] Improve navbar

---
 includes/sys_menu.php                         | 49 ++---------
 includes/sys_template.php                     | 49 +++--------
 includes/view/ShiftsFilterRenderer.php        |  2 +-
 includes/view/UserHintsRenderer.php           | 61 ++++++-------
 resources/assets/js/forms.js                  |  7 ++
 resources/assets/themes/base.scss             |  1 +
 resources/assets/themes/colored-links.scss    | 13 +++
 resources/views/layouts/parts/navbar.twig     | 85 +++++++++++++------
 src/Renderer/Twig/Extensions/Legacy.php       |  2 +-
 .../Renderer/Twig/Extensions/LegacyTest.php   |  2 +-
 10 files changed, 129 insertions(+), 142 deletions(-)
 create mode 100644 resources/assets/themes/colored-links.scss

diff --git a/includes/sys_menu.php b/includes/sys_menu.php
index 135179f9..7dcb3c91 100644
--- a/includes/sys_menu.php
+++ b/includes/sys_menu.php
@@ -23,9 +23,9 @@ function header_render_hints()
 {
     $user = auth()->user();
 
-    $hints_renderer = new UserHintsRenderer();
-
     if ($user) {
+        $hints_renderer = new UserHintsRenderer();
+
         $hints_renderer->addHint(admin_new_questions());
         $hints_renderer->addHint(user_angeltypes_unconfirmed_hint());
         $hints_renderer->addHint(render_user_departure_date_hint());
@@ -36,9 +36,11 @@ function header_render_hints()
         $hints_renderer->addHint(render_user_arrived_hint(), true);
         $hints_renderer->addHint(render_user_tshirt_hint(), true);
         $hints_renderer->addHint(render_user_dect_hint(), true);
+
+        return $hints_renderer->render();
     }
 
-    return $hints_renderer->render();
+    return '';
 }
 
 /**
@@ -51,39 +53,6 @@ function current_page()
     return request()->query->get('p') ?: str_replace('-', '_', request()->path());
 }
 
-/**
- * @return array
- */
-function make_user_submenu()
-{
-    $page = current_page();
-    $user_submenu = make_language_select();
-
-    if (auth()->can('user_settings') || auth()->can('logout')) {
-        $user_submenu[] = toolbar_dropdown_item_divider();
-    }
-
-    if (auth()->can('user_settings')) {
-        $user_submenu[] = toolbar_dropdown_item(
-            page_link_to('settings/profile'),
-            __('Settings'),
-            $page == 'settings/profile',
-            'person-fill-gear'
-        );
-    }
-
-    if (auth()->can('logout')) {
-        $user_submenu[] = toolbar_dropdown_item(
-            page_link_to('logout'),
-            __('Logout'),
-            $page == 'logout',
-            'box-arrow-left',
-        );
-    }
-
-    return $user_submenu;
-}
-
 /**
  * @return string
  */
@@ -146,10 +115,10 @@ function make_navigation()
     }
 
     if (count($admin_menu) > 0) {
-        $menu[] = toolbar_dropdown('', __('Admin'), $admin_menu);
+        $menu[] = toolbar_dropdown(__('Admin'), $admin_menu);
     }
 
-    return '<ul class="navbar-nav mb-2 mb-lg-0">' . join("\n", $menu) . '</ul>';
+    return join("\n", $menu);
 }
 
 /**
@@ -195,7 +164,7 @@ function make_room_navigation($menu)
         $room_menu[] = toolbar_dropdown_item(room_link($room), $room->name, false, 'pin-map-fill');
     }
     if (count($room_menu) > 0) {
-        $menu[] = toolbar_dropdown('map-marker', __('Rooms'), $room_menu);
+        $menu[] = toolbar_dropdown(__('Rooms'), $room_menu);
     }
     return $menu;
 }
@@ -212,7 +181,7 @@ function make_language_select()
 
     $items = [];
     foreach (config('locales') as $locale => $name) {
-        $url = url($request->getPathInfo(), ['set-locale' => $locale]);
+        $url = url($request->getPathInfo(), [...$request->getQueryParams(), 'set-locale' => $locale]);
 
         $items[] = toolbar_dropdown_item(
             htmlspecialchars($url),
diff --git a/includes/sys_template.php b/includes/sys_template.php
index b5979913..49915baf 100644
--- a/includes/sys_template.php
+++ b/includes/sys_template.php
@@ -186,7 +186,7 @@ function toolbar_pills($items)
 function toolbar_item_link($href, $icon, $label, $active = false)
 {
     return '<li class="nav-item">'
-        . '<a class="nav-link ' . ($active ? 'active' : '') . '" href="' . $href . '">'
+        . '<a class="nav-link ' . ($active ? 'active" aria-current="page"' : '"') . ' href="' . $href . '">'
         . ($icon != '' ? '<span class="bi bi-' . $icon . '"></span> ' : '')
         . $label
         . '</a>'
@@ -196,12 +196,13 @@ function toolbar_item_link($href, $icon, $label, $active = false)
 function toolbar_dropdown_item(string $href, string $label, bool $active, string $icon = null): string
 {
     return strtr(
-        '<li><a class="dropdown-item{active}" href="{href}">{icon} {label}</a></li>',
+        '<li><a class="dropdown-item{active}"{aria} href="{href}">{icon} {label}</a></li>',
         [
             '{href}'   => $href,
             '{icon}'   => $icon === null ? '' : '<i class="bi bi-' . $icon . '"></i>',
             '{label}'  => $label,
-            '{active}' => $active ? ' active' : ''
+            '{active}' => $active ? ' active' : '',
+            '{aria}' => $active ? ' aria-current="page"' : ''
         ]
     );
 }
@@ -212,20 +213,19 @@ function toolbar_dropdown_item_divider(): string
 }
 
 /**
- * @param string $icon
  * @param string $label
  * @param array  $submenu
- * @param string $class
+ * @param bool   $active
  * @return string
  */
-function toolbar_dropdown($icon, $label, $submenu, $class = ''): string
+function toolbar_dropdown($label, $submenu, $active = false): string
 {
     $template = <<<EOT
 <li class="nav-item dropdown">
-    <a class="nav-link dropdown-toggle {class}" href="#" id="navbarDropdown" role="button" data-bs-toggle="dropdown" aria-expanded="false">
-        {icon} {label}
+    <a class="nav-link dropdown-toggle{class}" href="#" role="button" data-bs-toggle="dropdown" aria-expanded="false">
+        {label}
     </a>
-    <ul class="dropdown-menu" aria-labelledby="navbarDropdown">
+    <ul class="dropdown-menu">
         {submenu}
     </ul>
 </li>
@@ -234,42 +234,13 @@ EOT;
     return strtr(
         $template,
         [
-            '{class}'   => $class,
+            '{class}'   => $active ? ' active' : '',
             '{label}'   => $label,
-            '{icon}'    => empty($icon) ? '' : '<i class="bi ' . $icon . '"></i>',
             '{submenu}' => join("\n", $submenu)
         ]
     );
 }
 
-/**
- * @param string   $icon
- * @param string   $label
- * @param string[] $content
- * @param string   $class
- *
- * @return string
- */
-function toolbar_popover($icon, $label, $content, $class = '')
-{
-    $dom_id = md5(microtime() . $icon . $label);
-    return '<li class="nav-item nav-item--userhints d-flex align-items-center ' . $class . '">'
-        . '<a id="' . $dom_id . '" href="#" tabindex="0" class="nav-link">'
-        . ($icon ? icon($icon) : '')
-        . $label
-        . '<small class="bi bi-caret-down-fill"></small>'
-        . '</a>'
-        . '<script type="text/javascript">
-                new bootstrap.Popover(document.getElementById(\'' . $dom_id . '\'), {
-                    container: \'body\',
-                    html: true,
-                    content: \'' . addslashes(join('', $content)) . '\',
-                    placement: \'bottom\',
-                    customClass: \'popover--userhints\'
-                })
-            </script></li>';
-}
-
 /**
  * Generiert HTML Code für eine "Seite".
  * Fügt dazu die übergebenen Elemente zusammen.
diff --git a/includes/view/ShiftsFilterRenderer.php b/includes/view/ShiftsFilterRenderer.php
index 94cc7c9a..0e1761ac 100644
--- a/includes/view/ShiftsFilterRenderer.php
+++ b/includes/view/ShiftsFilterRenderer.php
@@ -54,7 +54,7 @@ class ShiftsFilterRenderer
                 $link = $page_link . '&shifts_filter_day=' . $day;
                 $day_dropdown_items[] = toolbar_item_link($link, '', $day);
             }
-            $toolbar[] = toolbar_dropdown('', $selected_day, $day_dropdown_items, 'active');
+            $toolbar[] = toolbar_dropdown($selected_day, $day_dropdown_items, true);
 
             if ($dashboardFilter) {
                 $toolbar[] = sprintf(
diff --git a/includes/view/UserHintsRenderer.php b/includes/view/UserHintsRenderer.php
index b409d14e..55c65595 100644
--- a/includes/view/UserHintsRenderer.php
+++ b/includes/view/UserHintsRenderer.php
@@ -9,28 +9,6 @@ class UserHintsRenderer
 
     private $important = false;
 
-    /**
-     * Render the added hints to a popover for the toolbar.
-     *
-     * @return string
-     */
-    public function render()
-    {
-        if (count($this->hints) > 0) {
-            $hint_class = $this->important ? 'danger' : 'info';
-            $icon = $this->important ? 'exclamation-triangle' : 'info-circle';
-
-            return toolbar_popover(
-                $icon . ' text-white',
-                '',
-                $this->hints,
-                'bg-' . $hint_class
-            );
-        }
-
-        return '';
-    }
-
     /**
      * Add a hint to the list, if its not null and a not empty string.
      *
@@ -50,22 +28,35 @@ class UserHintsRenderer
     }
 
     /**
-     * Get all hints.
+     * Render the added hints to a popover for the toolbar.
      *
-     * @return string[]
+     * @return string
      */
-    public function getHints()
+    public function render()
     {
-        return $this->hints;
-    }
+        if (count($this->hints) > 0) {
+            $class_hint = $this->important ? 'danger' : 'info';
+            $icon = $this->important ? 'exclamation-triangle' : 'info-circle';
+            $data_bs_attributes = [
+                'toggle'       => 'popover',
+                'container'    => 'body',
+                'placement'    => 'bottom',
+                'custom-class' => 'popover--userhints',
+                'html'         => 'true',
+                'content'      => htmlspecialchars(join('', $this->hints)),
+            ];
+            $attr = '';
+            foreach ($data_bs_attributes as $attr_key => $attr_value) {
+                $attr .= ' data-bs-' . $attr_key . '="' . $attr_value . '"';
+            }
 
-    /**
-     * Are there important hints? This leads to a more intensive icon.
-     *
-     * @return bool
-     */
-    public function isImportant()
-    {
-        return $this->important;
+            return '<li class="nav-item nav-item--userhints d-flex align-items-center bg-' . $class_hint . '">'
+                . '<a class="nav-link dropdown-toggle text-light" href="#" role="button"' . $attr . '>'
+                . icon($icon)
+                . '</a>'
+                . '</li>';
+        }
+
+        return '';
     }
 }
diff --git a/resources/assets/js/forms.js b/resources/assets/js/forms.js
index e6ee68d9..3a7e46d7 100644
--- a/resources/assets/js/forms.js
+++ b/resources/assets/js/forms.js
@@ -247,6 +247,13 @@ ready(() => {
   });
 });
 
+/**
+ * Init Bootstrap Popover
+ */
+ready(() => {
+  document.querySelectorAll('[data-bs-toggle="popover"]').forEach((element) => new bootstrap.Popover(element));
+});
+
 /**
  * Show oauth buttons on welcome title click
  */
diff --git a/resources/assets/themes/base.scss b/resources/assets/themes/base.scss
index d0d6963f..00c51d0a 100644
--- a/resources/assets/themes/base.scss
+++ b/resources/assets/themes/base.scss
@@ -55,6 +55,7 @@ $form-label-font-weight: $font-weight-bold;
 @import 'choices';
 @import 'error';
 @import 'barchart';
+@import 'colored-links';
 
 $navbar-height: 3.125rem;
 
diff --git a/resources/assets/themes/colored-links.scss b/resources/assets/themes/colored-links.scss
new file mode 100644
index 00000000..7a858402
--- /dev/null
+++ b/resources/assets/themes/colored-links.scss
@@ -0,0 +1,13 @@
+// https://github.com/twbs/bootstrap/blob/main/scss/helpers/_colored-links.scss
+@if $link-shade-percentage != 0 {
+  @each $color, $value in $theme-colors {
+    a:hover > .text-#{'' + $color},
+    a:focus > .text-#{'' + $color} {
+      color: if(
+        color-contrast($value) == $color-contrast-light,
+        shade-color($value, $link-shade-percentage),
+        tint-color($value, $link-shade-percentage)
+      ) !important; // stylelint-disable-line declaration-no-important
+    }
+  }
+}
diff --git a/resources/views/layouts/parts/navbar.twig b/resources/views/layouts/parts/navbar.twig
index d0cd6067..a5c7ab0a 100644
--- a/resources/views/layouts/parts/navbar.twig
+++ b/resources/views/layouts/parts/navbar.twig
@@ -1,15 +1,28 @@
 {% import 'macros/base.twig' as m %}
 
-{% macro toolbar_item(label, link, active_page, icon) %}
+{% macro toolbar_item(label, link, active_page, icon, title) %}
     <li class="nav-item">
-        <a class="nav-link{% if page() == active_page %} active{% endif %}" href="{{ link }}">
+        <a
+            class="nav-link{% if page() == active_page %} active{% endif %}"
+            {% if page() == active_page %}aria-current="page"{% endif %}
+            {% if title %}title="{{ title }}"{% endif %}
+            href="{{ link }}"
+        >
             {% if icon %}{{ m.icon(icon) }}{% endif %}
             {{ label|raw }}
         </a>
     </li>
 {% endmacro %}
 
-<nav class="navbar fixed-top navbar-expand-lg border-bottom {{ theme['navbar_classes'] }}">
+{% macro dropdown_item(label, link, active_page, icon) %}
+    <li>
+        <a class="dropdown-item{% if page() == active_page %} active{% endif %}"{% if page() == active_page %} aria-current="page"{% endif %} href="{{ link }}">
+            {{ icon }} {{ label }}
+        </a>
+    </li>
+{% endmacro %}
+
+<nav class="navbar fixed-top navbar-expand-xxl border-bottom {{ theme['navbar_classes'] }}">
     <div class="container-fluid">
         <a class="navbar-brand" href="{{ url('/') }}">
             <span class="icon-icon_angel"></span>
@@ -19,7 +32,9 @@
             <span class="navbar-toggler-icon"></span>
         </button>
         <div class="collapse navbar-collapse" id="navbarSupportedContent">
-            {{ menu() }}
+            <ul class="navbar-nav mb-2 mb-lg-0">
+                {{ menu() }}
+            </ul>
 
             {% if config('header_items') %}
                 <ul class="navbar-nav mb-2 mb-lg-0">
@@ -33,36 +48,56 @@
                 </ul>
             {% endif %}
             <ul class="navbar-nav ms-auto mb-2 mb-lg-0">
+                {% if is_guest() %}
+                    {% if has_permission_to('register') and config('registration_enabled') %}
+                        {{ _self.toolbar_item(__('Register'), url('register'), 'register', 'plus') }}
+                    {% endif %}
+
+                    {% if has_permission_to('login') %}
+                        {{ _self.toolbar_item(__('login.login'), url('login'), 'login', 'box-arrow-in-right') }}
+                    {% endif %}
+                {% endif %}
+
                 {% if is_user() %}
-                    {{ _self.toolbar_item(menuUserShiftState(user), url('shifts', {'action': 'next'}), '', 'clock') }}
-                {% elseif has_permission_to('register') and config('registration_enabled') %}
-                    {{ _self.toolbar_item(__('Register'), url('register'), 'register', 'plus') }}
-                {% endif %}
+                    {{ _self.toolbar_item(menuUserShiftState(user), url('shifts', {'action': 'next'}), '', 'clock', __('Next shift')) }}
 
-                {% if has_permission_to('login') %}
-                    {{ _self.toolbar_item(__('login.login'), url('login'), 'login', 'box-arrow-in-right') }}
-                {% endif %}
+                    {% if has_permission_to('user_messages') %}
+                        {{ _self.toolbar_item(
+                            user_messages ? '<span class="badge bg-danger">' ~ user_messages ~ '</span>' : '',
+                            url('messages'),
+                            'messages',
+                            'envelope'
+                        ) }}
+                    {% endif %}
 
-                {% if is_user() and has_permission_to('user_messages') %}
-                    {{ _self.toolbar_item(
-                        user_messages ? '<span class="badge bg-danger">' ~ user_messages ~ '</span>' : '',
-                        url('messages'),
-                        'messages',
-                        'envelope'
-                    ) }}
-                {% endif %}
+                    {{ menuUserHints() }}
 
-                {{ menuUserHints() }}
+                    <li class="nav-item dropdown">
+                        <a class="nav-link dropdown-toggle" href="#" role="button" data-bs-toggle="dropdown" aria-expanded="false">
+                            {{ m.angel() }} {{ user.name }}
+                        </a>
+                        <ul class="dropdown-menu dropdown-menu-end">
+                            {% if has_permission_to('user_myshifts') %}
+                                {{ _self.dropdown_item(__('My shifts'), url('users', {'action': 'view'}), 'users', m.icon('calendar-range')) }}
+                            {% endif %}
 
-                {% if has_permission_to('user_myshifts') %}
-                    {{ _self.toolbar_item(user.name, url('users', {'action': 'view'}), 'users', 'icon icon-icon_angel') }}
+                            {% if has_permission_to('user_settings') %}
+                                {{ _self.dropdown_item(__('Settings'), url('settings/profile'), 'settings/profile', m.icon('person-fill-gear')) }}
+                            {% endif %}
+
+                            {% if has_permission_to('logout') %}
+                                {{ _self.dropdown_item(__('Logout'), url('logout'), 'logout', m.icon('box-arrow-left')) }}
+                            {% endif %}
+                        </ul>
+                    </li>
                 {% endif %}
 
                 <li class="nav-item dropdown">
-                    <a class="nav-link dropdown-toggle" href="#" id="navbarDropdown" role="button" data-bs-toggle="dropdown" aria-expanded="false">
+                    <a class="nav-link dropdown-toggle" href="#" role="button" data-bs-toggle="dropdown" aria-expanded="false">
+                        {{ m.icon('translate') }}
                     </a>
-                    <ul class="dropdown-menu dropdown-menu-end" aria-labelledby="navbarDropdown">
-                        {{ menuUserSubmenu()|join(" ")|raw }}
+                    <ul class="dropdown-menu dropdown-menu-end">
+                        {{ menuLanguages()|join(" ")|raw }}
                     </ul>
                 </li>
             </ul>
diff --git a/src/Renderer/Twig/Extensions/Legacy.php b/src/Renderer/Twig/Extensions/Legacy.php
index 91f65d05..43e68a9a 100644
--- a/src/Renderer/Twig/Extensions/Legacy.php
+++ b/src/Renderer/Twig/Extensions/Legacy.php
@@ -22,7 +22,7 @@ class Legacy extends TwigExtension
             new TwigFunction('menu', 'make_navigation', $isSafeHtml),
             new TwigFunction('menuUserShiftState', 'User_shift_state_render', $isSafeHtml),
             new TwigFunction('menuUserHints', 'header_render_hints', $isSafeHtml),
-            new TwigFunction('menuUserSubmenu', 'make_user_submenu', $isSafeHtml),
+            new TwigFunction('menuLanguages', 'make_language_select', $isSafeHtml),
             new TwigFunction('page', [$this, 'getPage']),
             new TwigFunction('msg', 'msg', $isSafeHtml),
         ];
diff --git a/tests/Unit/Renderer/Twig/Extensions/LegacyTest.php b/tests/Unit/Renderer/Twig/Extensions/LegacyTest.php
index cbd26e7f..18fd6018 100644
--- a/tests/Unit/Renderer/Twig/Extensions/LegacyTest.php
+++ b/tests/Unit/Renderer/Twig/Extensions/LegacyTest.php
@@ -23,7 +23,7 @@ class LegacyTest extends ExtensionTest
         $this->assertExtensionExists('menu', 'make_navigation', $functions, $isSafeHtml);
         $this->assertExtensionExists('menuUserShiftState', 'User_shift_state_render', $functions, $isSafeHtml);
         $this->assertExtensionExists('menuUserHints', 'header_render_hints', $functions, $isSafeHtml);
-        $this->assertExtensionExists('menuUserSubmenu', 'make_user_submenu', $functions, $isSafeHtml);
+        $this->assertExtensionExists('menuLanguages', 'make_language_select', $functions, $isSafeHtml);
         $this->assertExtensionExists('page', [$extension, 'getPage'], $functions);
         $this->assertExtensionExists('msg', 'msg', $functions, $isSafeHtml);
     }