IBX-650: redesign menu #1808

lucasOsti · 2021-07-14T10:29:24Z

Question	Answer
Tickets	https://issues.ibexa.co/browse/IBX-650
Related PR's	https://github.com/ezsystems/ezplatform-personalization/pull/215
	https://github.com/ezsystems/ezplatform-form-builder/pull/291
	https://github.com/ezsystems/ezcommerce-shop/pull/329
	https://github.com/ezsystems/ezplatform-page-builder/pull/791
	https://github.com/ezsystems/ezplatform-icons/pull/7
	https://github.com/ezsystems/ezplatform-site-factory/pull/129
Bug fix?	no
New feature?	yes
BC breaks?	yes
Tests pass?	no
Doc needed?	??
License	GPL-2.0

New look:

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

dew326 · 2021-07-20T11:47:10Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+            return;
+        }
+
+        allPopups.push(


You are not using those popups. Why do you keep them in array?

src/bundle/Resources/public/js/scripts/admin.notifications.modal.js

GrabowskiM · 2021-07-21T12:32:33Z

src/bundle/Resources/public/js/scripts/helpers/tooltips.helper.js

-                const extraClasses = tooltipNode.dataset.extraClasses || '';
-                const placement = tooltipNode.dataset.placement || 'bottom';
+                const extraClass = tooltipNode.dataset.tooltipExtraClass || '';
+                const placement = tooltipNode.dataset.tooltipPlacement || 'bottom';


Suggested change

const placement = tooltipNode.dataset.tooltipPlacement || 'bottom';

const placement = tooltipNode.dataset.tooltipPlacement ?? 'bottom';

GrabowskiM · 2021-07-21T12:32:41Z

src/bundle/Resources/public/js/scripts/helpers/tooltips.helper.js

@@ -36,8 +36,8 @@
                    show: tooltipNode.dataset.delayShow || 150,
                    hide: tooltipNode.dataset.delayHide || 75,
                };
-                const extraClasses = tooltipNode.dataset.extraClasses || '';
-                const placement = tooltipNode.dataset.placement || 'bottom';
+                const extraClass = tooltipNode.dataset.tooltipExtraClass || '';


Suggested change

const extraClass = tooltipNode.dataset.tooltipExtraClass || '';

const extraClass = tooltipNode.dataset.tooltipExtraClass ?? '';

GrabowskiM · 2021-07-21T12:33:54Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

@@ -0,0 +1,132 @@
+(function(global, doc, localStorage, eZ) {


Suggested change

(function(global, doc, localStorage, eZ) {

(function(global, doc, eZ, localStorage) {

nitpicking, but so far eZ (if it was used) was always third argument

GrabowskiM · 2021-07-21T12:34:56Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+    let resizeStartPositionX = 0;
+    let secondMenuLevelCurrentWidth = secondLevelMenuNode.getBoundingClientRect().width;
+    const showSecondLevelMenu = (event) => {
+        const { toggle } = event.currentTarget.dataset;


Imho there's no need to put it into new variable if you use it only in one place (in if, in this case)

So you can remove it now ;)

GrabowskiM · 2021-07-21T12:47:06Z

src/bundle/Resources/views/themes/admin/ui/layout.html.twig

+            <div class="ibexa-main-header">
+                <div class="ibexa-main-header__brand-column">
+                    <a class="ibexa-main-header__brand" href="{{ url('ezplatform.dashboard') }}">
+                        <img class="ibexa-main-header__brand-image" src="/bundles/ezplatformadminui/img/ibexa-logo.svg" alt="Ibexa" />


hmmm... translation? :D

Also, I'm not sure, don't we have some func for resources path?

GrabowskiM · 2021-07-21T12:47:30Z

src/bundle/Resources/views/themes/admin/ui/menu/main.html.twig

+    {% set current_item = item %}
+
+    {% for item in current_item.children %}
+        {% if matcher.isCurrent(item) and item.children|length == 0%}


Suggested change

{% if matcher.isCurrent(item) and item.children|length == 0%}

{% if matcher.isCurrent(item) and item.children|length == 0 %}

GrabowskiM · 2021-07-21T12:49:16Z

src/bundle/Resources/views/themes/admin/ui/menu/main_2nd_level.html.twig

+    {{ block('linkElement') }}
+{% endblock %}
+
+


src/bundle/Resources/views/themes/admin/ui/menu/main_base.html.twig

GrabowskiM · 2021-07-21T12:50:06Z

src/bundle/Resources/views/themes/admin/ui/menu/main_base.html.twig

+        {%- endif -%}
+
+        {%- if item.extras.template is defined -%}
+            {% include(item.extras.template)  %}


Suggested change

{% include(item.extras.template) %}

{% include(item.extras.template) %}

GrabowskiM · 2021-07-21T13:30:33Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+    let resizeStartPositionX = 0;
+    let secondMenuLevelCurrentWidth = secondLevelMenuNode.getBoundingClientRect().width;
+    const showSecondLevelMenu = (event) => {
+        const { toggle } = event.currentTarget.dataset;


So you can remove it now ;)

tischsoic · 2021-07-19T14:11:54Z

src/bundle/Resources/public/js/scripts/admin.user.menu.js

+    const triggerElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');
+    const popupMenuElement = userMenuContainer.querySelector('.ibexa-popup-menu');
+    const popupMenu = new eZ.core.PopupMenu({
+        triggerElement,
+        popupMenuElement,


Suggested change

const triggerElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

const popupMenuElement = userMenuContainer.querySelector('.ibexa-popup-menu');

const popupMenu = new eZ.core.PopupMenu({

triggerElement,

popupMenuElement,

const usernameElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

const popupMenuElement = userMenuContainer.querySelector('.ibexa-popup-menu');

const popupMenu = new eZ.core.PopupMenu({

triggerElement: usernameElement,

popupMenuElement,

tischsoic · 2021-07-19T14:12:22Z

src/bundle/Resources/public/js/scripts/admin.user.menu.js

    }
+
+    const triggerElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');


What does name from CSS selector refer to? Is it a username?

tischsoic · 2021-07-22T06:37:17Z

src/bundle/Resources/public/scss/mixins/_main-menu.scss

@@ -0,0 +1,8 @@
+@mixin hidden-main-menu-node {


I'm not sure whether we really need this as a global mixin, it can be local to .ibexa-main-menu as it is used only there. What do you think?

tischsoic · 2021-07-22T09:28:55Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+        if (!localStorage.getItem('secondLevelMenuWidth') || secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--hidden')) {
+            return;
+        }
+
+        const { secondLevelMenuWidth } = localStorage;


Suggested change

if (!localStorage.getItem('secondLevelMenuWidth') || secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--hidden')) {

return;

}

const { secondLevelMenuWidth } = localStorage;

const savedSecondLevelMenuWidth = localStorage.getItem('secondLevelMenuWidth');

const isSecondLevelMenuHidden = secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--hidden');

if (!savedSecondLevelMenuWidth || isSecondLevelMenuHidden) {

return;

}

tischsoic · 2021-07-22T09:30:43Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+        const newMenuWidth = secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--collapsed')
+            ? SECOND_LEVEL_EXPANDED_WIDTH
+            : SECOND_LEVEL_COLLAPSED_WIDTH;


Suggested change

const newMenuWidth = secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--collapsed')

? SECOND_LEVEL_EXPANDED_WIDTH

: SECOND_LEVEL_COLLAPSED_WIDTH;

const isSecondLevelMenuCollapsed = secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--collapsed');

const newMenuWidth = isSecondLevelMenuCollapsed ? SECOND_LEVEL_EXPANDED_WIDTH : SECOND_LEVEL_COLLAPSED_WIDTH;

tischsoic · 2021-07-22T09:34:15Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+        setWidthOfSecondLevelMenu();
+    };
+    const fitMenu = () => {
+        const diff = Math.max(HEADER_HEIGHT - global.scrollY, 0);


IMO name of this variable is too generic.

tischsoic · 2021-07-22T09:35:31Z

src/bundle/Resources/public/js/scripts/sidebar/main.menu.js

+        firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.top = `${diff}px`;
+        firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.height = `${global.innerHeight - diff}px`;


Suggested change

firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.top = `${diff}px`;

firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.height = `${global.innerHeight - diff}px`;

const firstLevelMenuItemsList = firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list');

firstLevelMenuItemsList.style.top = `${diff}px`;

firstLevelMenuItemsList.style.height = `${global.innerHeight - diff}px`;

tischsoic · 2021-07-22T09:39:10Z

src/bundle/Resources/public/scss/_header-user-menu.scss

+
+        &:hover {
+            cursor: pointer;
+        }


Element is clickable all the time, not only on hover.

Suggested change

&:hover {

cursor: pointer;

}

cursor: pointer;

dew326 · 2021-07-22T13:21:00Z

src/bundle/Resources/public/js/scripts/admin.user.menu.js

    }
+
+    const usernameElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');


Suggested change

const usernameElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

const userNameElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

lserwatka · 2021-07-26T09:49:56Z

Rebase is required here. Can this be merged once Dashboard is in?

mnocon · 2021-07-26T09:52:19Z

@lserwatka this one requires some changes in Behat tests that I'd like to do before it's merged, I will be working on it soon

lserwatka · 2021-07-26T09:54:17Z

@mnocon alright, thank you for clarification 👍

sonarcloud · 2021-07-29T11:14:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
2.1% Duplication

lucasOsti marked this pull request as draft July 15, 2021 08:50

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from e41465f to 4676046 Compare July 19, 2021 10:09

lucasOsti requested review from dew326, tischsoic and GrabowskiM July 19, 2021 10:10

lucasOsti marked this pull request as ready for review July 19, 2021 10:10

lucasOsti removed request for tischsoic, dew326 and GrabowskiM July 19, 2021 10:50

lucasOsti marked this pull request as draft July 19, 2021 10:50

lucasOsti requested review from tischsoic, GrabowskiM and dew326 July 19, 2021 12:55

lucasOsti marked this pull request as ready for review July 19, 2021 12:55

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 786e29e to f6590a7 Compare July 19, 2021 13:26

dew326 reviewed Jul 20, 2021

View reviewed changes

lucasOsti requested a review from dew326 July 20, 2021 12:14

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 614a08b to b67a08c Compare July 21, 2021 10:06

GrabowskiM requested changes Jul 21, 2021

View reviewed changes

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 9a99a87 to 999f8a9 Compare July 21, 2021 13:20

lucasOsti requested a review from GrabowskiM July 21, 2021 13:21

GrabowskiM requested changes Jul 21, 2021

View reviewed changes

lucasOsti requested a review from GrabowskiM July 21, 2021 13:35

GrabowskiM approved these changes Jul 21, 2021

View reviewed changes

dew326 approved these changes Jul 22, 2021

View reviewed changes

tischsoic requested changes Jul 22, 2021

View reviewed changes

lucasOsti requested review from dew326 and tischsoic July 22, 2021 12:44

tischsoic approved these changes Jul 22, 2021

View reviewed changes

dew326 requested changes Jul 22, 2021

View reviewed changes

lucasOsti requested a review from dew326 July 22, 2021 13:23

dew326 approved these changes Jul 22, 2021

View reviewed changes

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 6b58589 to 706d487 Compare July 23, 2021 08:29

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from d9cc2a2 to 32d468b Compare July 26, 2021 09:55

IBX-650: redesign menu

f889b19

lucasOsti force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 7e45537 to f889b19 Compare July 28, 2021 10:28

After rebase

7af16fd

mnocon mentioned this pull request Jul 28, 2021

Added ElementAttributeCriterion ezsystems/BehatBundle#197

Closed

mnocon and others added 7 commits July 29, 2021 13:04

[Behat] Adapted tests to the new navigation

5921813

Behat

3f09015

Behat

5fb35b5

Behat

413c6c6

[Behat] Adjusted selectors to redesign

4825fb1

Fixed behat tests

63a1be2

[Behat] Improved stability

72ae216

mnocon mentioned this pull request Jul 29, 2021

IBX-650: Added ElementAttributeCriterion ezsystems/BehatBundle#199

Merged

mnocon force-pushed the IBX-650-As-an-Editor-I-want-to-see-the-redesigned-navigation-menu branch from 0a6ffc4 to 72ae216 Compare July 29, 2021 11:11

dew326 merged commit 3e732b0 into ezsystems:master Jul 29, 2021

ViniTou mentioned this pull request Sep 2, 2021

IBX-151: Removed previously dropped classes #1883

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-650: redesign menu #1808

IBX-650: redesign menu #1808

lucasOsti commented Jul 14, 2021 •

edited

Loading

dew326 Jul 20, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

GrabowskiM Jul 21, 2021

tischsoic Jul 19, 2021

tischsoic Jul 19, 2021

tischsoic Jul 22, 2021

tischsoic Jul 22, 2021

tischsoic Jul 22, 2021

tischsoic Jul 22, 2021

tischsoic Jul 22, 2021

tischsoic Jul 22, 2021

dew326 Jul 22, 2021

lserwatka commented Jul 26, 2021

mnocon commented Jul 26, 2021

lserwatka commented Jul 26, 2021

sonarcloud bot commented Jul 29, 2021

	const placement = tooltipNode.dataset.tooltipPlacement \|\| 'bottom';
	const placement = tooltipNode.dataset.tooltipPlacement ?? 'bottom';

	const extraClass = tooltipNode.dataset.tooltipExtraClass \|\| '';
	const extraClass = tooltipNode.dataset.tooltipExtraClass ?? '';

	(function(global, doc, localStorage, eZ) {
	(function(global, doc, eZ, localStorage) {

	{% if matcher.isCurrent(item) and item.children\|length == 0%}
	{% if matcher.isCurrent(item) and item.children\|length == 0 %}

	{% include(item.extras.template) %}
	{% include(item.extras.template) %}

		}

		const triggerElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

-        if (!localStorage.getItem('secondLevelMenuWidth') || secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--hidden')) {
-            return;
-        }
-        const { secondLevelMenuWidth } = localStorage;
+        const savedSecondLevelMenuWidth = localStorage.getItem('secondLevelMenuWidth');
+        const isSecondLevelMenuHidden = secondLevelMenuNode.classList.contains('ibexa-main-menu__navbar--hidden');
+        if (!savedSecondLevelMenuWidth || isSecondLevelMenuHidden) {
+            return;
+        }

		firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.top = `${diff}px`;
		firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.height = `${global.innerHeight - diff}px`;

-        firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.top = `${diff}px`;
-        firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list').style.height = `${global.innerHeight - diff}px`;
+        const firstLevelMenuItemsList = firstLevelMenuNode.querySelector('.ibexa-main-menu__items-list');
+        firstLevelMenuItemsList.style.top = `${diff}px`;
+        firstLevelMenuItemsList.style.height = `${global.innerHeight - diff}px`;

		}

		const usernameElement = userMenuContainer.querySelector('.ibexa-header-user-menu__name');

IBX-650: redesign menu #1808

IBX-650: redesign menu #1808

Conversation

lucasOsti commented Jul 14, 2021 • edited Loading

Checklist:

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

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

lserwatka commented Jul 26, 2021

mnocon commented Jul 26, 2021

lserwatka commented Jul 26, 2021

sonarcloud bot commented Jul 29, 2021

lucasOsti commented Jul 14, 2021 •

edited

Loading