IBX-976: Disabled transition for side menu after reload page #1893

lucasOsti · 2021-09-09T13:14:44Z

Question	Answer
Tickets	https://issues.ibexa.co/browse/IBX-976
Bug fix?	no
New feature?	no
BC breaks?	no
Tests pass?	no
Doc needed?	no
License	GPL-2.0

Checklist:

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

dew326 · 2021-09-10T06:46:58Z

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

+        const expiryDaysInMiliseconds = expiryDays * 24 * 60 * 60 * 1000;
+
+        date.setTime(date.getTime() + expiryDaysInMiliseconds);
+        doc.cookie = `${name}=${value};expires=${date.toUTCString()};path=${path}`;


Maybe it would be better to use max-age instead of expires?

dew326 · 2021-09-10T06:54:03Z

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

+        doc.cookie = `${name}=${value};expires=${date.toUTCString()};path=${path}`;
+    };
+    const getCookie = (name) => {
+        let cookieName = name + '=';


Suggested change

let cookieName = name + '=';

const cookieName = name + '=';

dew326 · 2021-09-10T06:54:11Z

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

+    };
+    const getCookie = (name) => {
+        let cookieName = name + '=';
+        let decodedCookie = decodeURIComponent(doc.cookie);


Suggested change

let decodedCookie = decodeURIComponent(doc.cookie);

const decodedCookie = decodeURIComponent(doc.cookie);

dew326 · 2021-09-10T06:54:21Z

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

+    const getCookie = (name) => {
+        let cookieName = name + '=';
+        let decodedCookie = decodeURIComponent(doc.cookie);
+        let cookiesArray = decodedCookie.split(';');


Suggested change

let cookiesArray = decodedCookie.split(';');

const cookiesArray = decodedCookie.split(';');

dew326 · 2021-09-10T06:55:50Z

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

+        let cookieValue = null;
+
+        cookiesArray.forEach((cookie) => {
+            cookie = cookie.trim();


here is a lot of assignment for cookie which is function param, I don't think this is best practice.

dew326 · 2021-09-10T06:57:10Z

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

@@ -1,6 +1,11 @@
 {% extends '@ezdesign/ui/menu/main_base.html.twig' %}

 {% block root %}
+    {% set resizerWidth = 10 %}


I think it should snake not camelCase in twig.

dew326 · 2021-09-10T09:29:11Z

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

+        const decodedCookie = decodeURIComponent(doc.cookie);
+        const cookiesArray = decodedCookie.split(';');
+
+        for (index in cookiesArray) {


I think for ... of would be better.

Maybe even change it all to .find

const cookieValue = cookiesArray.find((cookie) => { const cookieString = cookie.trim(); return cookieString.indexOf(cookieName) === 0; }); return cookieValue ? cookieValue.split('=')[1] : '';

or sth like this

GrabowskiM · 2021-09-10T11:03:29Z

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

+        const decodedCookie = decodeURIComponent(doc.cookie);
+        const cookiesArray = decodedCookie.split(';');
+
+        for (index in cookiesArray) {


Maybe even change it all to .find

const cookieValue = cookiesArray.find((cookie) => { const cookieString = cookie.trim(); return cookieString.indexOf(cookieName) === 0; }); return cookieValue ? cookieValue.split('=')[1] : '';

or sth like this

tischsoic · 2021-09-10T11:05:09Z

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

+            }
+        }
+
+        return '';


I think that no value should be represented by null.

tischsoic · 2021-09-10T11:08:53Z

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

+        doc.cookie = `${name}=${value};max-age=${maxAge};path=${path}`;
+    };
+    const getCookie = (name) => {
+        const cookieName = name + '=';


nitpick: this is actually not a cookie name, but cookie name with equal sign.
Personally, because it is very short and straightforward, I would prefer to remove this and just add `${name}=` inside cookieString.indexOf.

tischsoic · 2021-09-10T11:09:14Z

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

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


Suggested change

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

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

tischsoic · 2021-09-10T11:12:23Z

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

+    {% set second_level_manual_resize_min_width = 80 %}
+    {% set second_level_menu_width = app.request.cookies.get('second_menu_width') %}
+    {% set second_menu_width_style = '' %}
+    {% set second_menu_list_widt_style = '' %}


typo

Suggested change

{% set second_menu_list_widt_style = '' %}

{% set second_menu_list_width_style = '' %}

sonarcloud · 2021-09-10T12:00:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.1% Duplication

lucasOsti force-pushed the IBX-976-Disabled-transition-after-reload-side-menu branch from 999dcd6 to 05c7619 Compare September 9, 2021 13:31

lucasOsti requested review from GrabowskiM, tischsoic and dew326 September 9, 2021 13:32

lucasOsti force-pushed the IBX-976-Disabled-transition-after-reload-side-menu branch from 05c7619 to d51a558 Compare September 9, 2021 16:02

dew326 requested changes Sep 10, 2021

View reviewed changes

lucasOsti requested a review from dew326 September 10, 2021 08:40

IBX-976: Disabled transition for side menu after reload page

a57a240

lucasOsti force-pushed the IBX-976-Disabled-transition-after-reload-side-menu branch from b3ae836 to a57a240 Compare September 10, 2021 09:21

dew326 requested changes Sep 10, 2021

View reviewed changes

lucasOsti requested a review from dew326 September 10, 2021 10:27

GrabowskiM requested changes Sep 10, 2021

View reviewed changes

tischsoic requested changes Sep 10, 2021

View reviewed changes

After CR

f582aa7

lucasOsti requested review from GrabowskiM and tischsoic September 10, 2021 12:38

dew326 approved these changes Sep 13, 2021

View reviewed changes

GrabowskiM approved these changes Sep 13, 2021

View reviewed changes

tischsoic approved these changes Sep 13, 2021

View reviewed changes

dew326 merged commit 8e67df8 into ezsystems:master Sep 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IBX-976: Disabled transition for side menu after reload page #1893

IBX-976: Disabled transition for side menu after reload page #1893

lucasOsti commented Sep 9, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

dew326 Sep 10, 2021

GrabowskiM Sep 10, 2021

GrabowskiM Sep 10, 2021

tischsoic Sep 10, 2021

tischsoic Sep 10, 2021

tischsoic Sep 10, 2021

tischsoic Sep 10, 2021

sonarcloud bot commented Sep 10, 2021

	let decodedCookie = decodeURIComponent(doc.cookie);
	const decodedCookie = decodeURIComponent(doc.cookie);

	let cookiesArray = decodedCookie.split(';');
	const cookiesArray = decodedCookie.split(';');

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

	{% set second_menu_list_widt_style = '' %}
	{% set second_menu_list_width_style = '' %}

IBX-976: Disabled transition for side menu after reload page #1893

IBX-976: Disabled transition for side menu after reload page #1893

Conversation

lucasOsti commented Sep 9, 2021

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

sonarcloud bot commented Sep 10, 2021