1650 bug nav collapsed state #1665

UlianaMunich · 2020-10-15T10:32:31Z

Fixes #1650

Extends the Core API UX part with a new function that allows collapsing the left side nav.

Docu and tests are attached

…with collapseLeftSideNav function

docs/luigi-core-api.md

core/src/navigation/services/semi-collapsed-navigation.js

…h/luigi into 1650-bug-nav-collapsed-state

JohannesDoberer

LGTM 👍 !

ndricimrr

👍

JohannesDoberer · 2020-10-26T12:45:21Z

core/src/navigation/services/semi-collapsed-navigation.js

@@ -7,11 +7,20 @@ class SemiCollapsibleNavigationClass {
    this.responsiveNavSetting = LuigiConfig.getConfigValue(
      'settings.responsiveNavigation'
    );
-    this.semiCollapsible =
+    let currentCollapsedState = localStorage.getItem(


My suggestion:

let currentCollapsedState = JSON.parse(localStorage.getItem( NavigationHelpers.COL_NAV_KEY ));

You will get string instead of a boolean value if we don't parse the return value for currentCollapsedState.

JohannesDoberer · 2020-10-26T12:48:44Z

core/src/navigation/services/semi-collapsed-navigation.js

+      this.responsiveNavSetting === 'Fiori3';
+
+    //checking if here was a previous state in localStorage before the reload
+    if (currentCollapsedState != false && getResponsiveNavSettings) {


I would remove the else part, because after a reload the variable this.semiCollapsible will be always undefined and this lead to problems if you resize from desktop to mobile.
Code could look like this:

//checking if here was a previous state in localStorage before the reload if (currentCollapsedState !== null && getResponsiveNavSettings) { this.isSemiCollapsed = this.getCollapsed(); } this.semiCollapsible = getResponsiveNavSettings ? true : false;

done, thanks for checking

stanleychh · 2020-10-26T13:25:29Z

core/src/navigation/services/semi-collapsed-navigation.js

@@ -10,16 +10,15 @@ class SemiCollapsibleNavigationClass {
    let currentCollapsedState = localStorage.getItem(
      NavigationHelpers.COL_NAV_KEY
    );
-    let checkTheResponsiveNavSettings =
+    let getResponsiveNavSettings =


It seems using const is more suitable. There is no updated afterward.
I would suggest name it with is- prefix. e.g. const isResponsiveNavSemiCollapsibleOrFiori3 something like this. It is easier to understand at first sight.

…h/luigi into 1650-bug-nav-collapsed-state

stanleychh · 2020-10-27T14:36:35Z

core/src/navigation/services/semi-collapsed-navigation.js

@@ -7,19 +7,23 @@ class SemiCollapsibleNavigationClass {
    this.responsiveNavSetting = LuigiConfig.getConfigValue(
      'settings.responsiveNavigation'
    );
-    let currentCollapsedState = localStorage.getItem(
-      NavigationHelpers.COL_NAV_KEY
+    let currentCollapsedState = JSON.parse(


This should be const as well.
Is this a boolean ? If so, could you please change to is prefix naming?

it is a string, Hannes mentioned it above, when suggested the changes

This should be a boolean after JSON.parse

right, mixed it up and now renamed to isCurrentStateCollapsed

stanleychh · 2020-10-27T14:38:02Z

core/src/navigation/services/semi-collapsed-navigation.js

      this.responsiveNavSetting === 'semiCollapsible' ||
      this.responsiveNavSetting === 'Fiori3';

    //checking if here was a previous state in localStorage before the reload
-    if (currentCollapsedState != false && getResponsiveNavSettings) {
+    if (
+      currentCollapsedState != false &&


Besides the naming, Please change to !==

stanleychh · 2020-10-27T14:45:00Z

core/src/navigation/services/semi-collapsed-navigation.js

+    this.semiCollapsible = isResponsiveNavSemiCollapsibleOrFiori3
+      ? true
+      : false;


Suggested change

this.semiCollapsible = isResponsiveNavSemiCollapsibleOrFiori3

? true

: false;

this.semiCollapsible = isResponsiveNavSemiCollapsibleOrFiori3;

Also, since semiCollapsible is same as isResponsiveNavSemiCollapsibleOrFiori3. Please check do we need this separate semiCollapsible variable?

luigi/core/src/navigation/services/semi-collapsed-navigation.js

Line 31 in 0739cfe

this.semiCollapsible &&

and

luigi/core/src/navigation/services/semi-collapsed-navigation.js

Line 50 in 0739cfe

semiCollapsible: this.semiCollapsible

thanks for double-checking, We need it for the mobile collapsed state as well, so that on mobile the left side nav always set to 'true' even after the reload. And setting this.semiCollapsible = isResponsiveNavSemiCollapsibleOrFiori3; doesn't give a chance to make it false.

So I'd rather leave it as it is. What do you think?

JohannesDoberer

Good Job!

* master: Update fiddle to 5.0.0 (SAP#1679) Docu fix (SAP#1681) Update contribution guidelines (SAP#1655) Luigi 1.5 release notes (SAP#1678) Release 1.5.0 (SAP#1677) App Loading Indicator bug in docu (SAP#1661) Bug nav collapsed state (SAP#1665) IE11 domain check (SAP#1671)

UlianaMunich added 4 commits October 14, 2020 10:47

fix the bug with collapsed mode of left side nav and extend Core API …

4d156fc

…with collapseLeftSideNav function

Add data-id to the left side nav for tests

24f1e9c

fix issue with right reload state check

f5e22c0

merge changes from master

6085081

UlianaMunich requested review from alexandra-simeonova, hardl, JohannesDoberer, maxmarkus, ndricimrr and stanleychh as code owners October 15, 2020 10:32

JohannesDoberer self-assigned this Oct 15, 2020

JohannesDoberer added the bug Something isn't working label Oct 15, 2020

JohannesDoberer added this to the Sprint 13 milestone Oct 15, 2020

JohannesDoberer requested changes Oct 16, 2020

View reviewed changes

docs/luigi-core-api.md Outdated Show resolved Hide resolved

JohannesDoberer reviewed Oct 19, 2020

View reviewed changes

core/src/navigation/services/semi-collapsed-navigation.js Outdated Show resolved Hide resolved

UlianaMunich and others added 3 commits October 19, 2020 12:07

fix merge conflicts and align the rules regarding collapsible state

4832cd9

fix issue with docu for collaapseLeftSideNav

f1f62e0

Merge branch 'master' into 1650-bug-nav-collapsed-state

8fecd41

ndricimrr self-assigned this Oct 20, 2020

UlianaMunich and others added 4 commits October 22, 2020 11:35

Merge branch 'master' into 1650-bug-nav-collapsed-state

bda9692

Merge branch 'master' into 1650-bug-nav-collapsed-state

5f64890

fix typo in docu

d4c47d5

Merge branch '1650-bug-nav-collapsed-state' of github.com:UlianaMunic…

d0e78c0

…h/luigi into 1650-bug-nav-collapsed-state

JohannesDoberer approved these changes Oct 22, 2020

View reviewed changes

ndricimrr approved these changes Oct 22, 2020

View reviewed changes

UlianaMunich added 2 commits October 23, 2020 12:31

Add Fiori3 collapsed state tests

49fc56f

Clean up the local storage check for collapsed state

bb0cdf0

JohannesDoberer reviewed Oct 26, 2020

View reviewed changes

JohannesDoberer requested changes Oct 26, 2020

View reviewed changes

Merge branch 'master' into 1650-bug-nav-collapsed-state

d015f12

stanleychh reviewed Oct 26, 2020

View reviewed changes

UlianaMunich added 2 commits October 27, 2020 14:13

refactor the syntax of semicollapsed navigationn

0739cfe

Merge branch '1650-bug-nav-collapsed-state' of github.com:UlianaMunic…

01a3df3

…h/luigi into 1650-bug-nav-collapsed-state

stanleychh reviewed Oct 27, 2020

View reviewed changes

chanbge the conditions rules in the function

fea8785

stanleychh self-assigned this Oct 28, 2020

UlianaMunich added 3 commits October 28, 2020 10:57

rename the variable

77850c6

refactor the collapsed statte IF function

a397d13

align fomatting

02a3477

JohannesDoberer approved these changes Oct 28, 2020

View reviewed changes

JohannesDoberer and others added 3 commits October 28, 2020 13:04

Merge branch 'master' into 1650-bug-nav-collapsed-state

1791a59

Merge branch 'master' into 1650-bug-nav-collapsed-state

8d21b81

Merge branch 'master' into 1650-bug-nav-collapsed-state

5c7a6a0

UlianaMunich merged commit 77c4e08 into SAP:master Oct 28, 2020

UlianaMunich deleted the 1650-bug-nav-collapsed-state branch October 28, 2020 14:24

ndricimrr mentioned this pull request Oct 28, 2020

Release 1.5.0 #1677

Merged

stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021

Bug nav collapsed state (SAP#1665)

0b825c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1650 bug nav collapsed state #1665

1650 bug nav collapsed state #1665

UlianaMunich commented Oct 15, 2020

JohannesDoberer left a comment

ndricimrr left a comment

JohannesDoberer Oct 26, 2020 •

edited

Loading

UlianaMunich Oct 27, 2020

JohannesDoberer Oct 26, 2020

UlianaMunich Oct 27, 2020

stanleychh Oct 26, 2020

UlianaMunich Oct 27, 2020

stanleychh Oct 27, 2020

UlianaMunich Oct 28, 2020 •

edited

Loading

stanleychh Oct 28, 2020

UlianaMunich Oct 28, 2020

stanleychh Oct 27, 2020

UlianaMunich Oct 28, 2020

stanleychh Oct 27, 2020 •

edited

Loading

UlianaMunich Oct 28, 2020

JohannesDoberer left a comment

1650 bug nav collapsed state #1665

1650 bug nav collapsed state #1665

Conversation

UlianaMunich commented Oct 15, 2020

JohannesDoberer left a comment

Choose a reason for hiding this comment

ndricimrr left a comment

Choose a reason for hiding this comment

JohannesDoberer Oct 26, 2020 • 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

UlianaMunich Oct 28, 2020 • 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

stanleychh Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohannesDoberer left a comment

Choose a reason for hiding this comment

JohannesDoberer Oct 26, 2020 •

edited

Loading

UlianaMunich Oct 28, 2020 •

edited

Loading

stanleychh Oct 27, 2020 •

edited

Loading