Nav collapsed feature #259

parostatkiem-zz · 2018-11-29T08:27:01Z

Description
I've also fixed the hideNav bug (some css rules weren't applied because somebody decided to use id instead of class attribute.

Changes proposed in this pull request:

introduce navCollapse property in Node configuration
describe it in the documentation
add unit & e2e tets
turn this feature on at /overview and an added example route in Angular example

Related issue(s)

Resolves #243

…avCollapsed-feature

dariadomagala-sap

Please also add it to the overview page 'Luigi Core Features' section in the angular example.

dariadomagala-sap · 2018-11-29T08:37:30Z

core/examples/luigi-sample-angular/e2e/tests/navigation.spec.js

@@ -115,5 +115,10 @@ describe('Navigation', () => {
        expect(loc.hash).to.eq('#/projects/pr2/dps/dps1');
      });
    });
+    it('should hide left Nav', () => {
+      cy.visit('/#/projects/pr1/navCollapse');
+      cy.get('.no-left-nav');


This line doesn't do anything.

It does. It checks if there is any element with no-left-nav class. It's important because this class moves our iframeContainer to the left edge. If you change the classname to anything else, Cypress won't be able to find it and the test will fail:

I would be also confused with this line. Maybe you could explicitly do the check, otherwise someone could also think this line does nothing, remove it and then the test will not check what it has to check. Maybe sth like this:
cy.get('.no-left-nav').should('exist');

dariadomagala-sap · 2018-11-29T08:42:09Z

core/examples/luigi-sample-angular/src/assets/extendedConfiguration.js

@@ -419,7 +425,8 @@ Luigi.setConfig({
        {
          pathSegment: 'overview',
          label: 'Overview',
-          viewUrl: '/sampleapp.html#/overview'
+          viewUrl: '/sampleapp.html#/overview',
+          navCollapse: true


I would leave the navigation here, it's enough to show it in the feature component. At the beginning I thought something was wrongly implemented that the navigation is not shown on the main page ;)

It would be fine for me to leave it, but then please add some left margin to the headline in the content area

dariadomagala-sap · 2018-11-29T08:43:25Z

core/src/App.html

@@ -17,8 +17,8 @@
 	</div>
 	{/if}

-	<TopNav pathData={navigationPath} />
-	<LeftNav pathData={navigationPath} />
+	<TopNav pathData={navigationPath} /> {#if !(hideNav||hideLeftNav)}


Please move those ifs to new lines so it's more readable.

dariadomagala-sap · 2018-11-29T08:55:19Z

docs/navigation-configuration.md

-  - **sameWindow** defines if the external URL is opened in a new or current tab.
-  - **url** is the external URL that the node leads to.
+- **externalLink** is an object which indicates that the node links to an external URL. If this parameter is defined, **pathSegment** and **link** parameters are ignored. It has the following properties:
+- **sameWindow** defines if the external URL is opened in a new or current tab.


Those additional spaces here were actually needed. Same one line below.

…avCollapsed-feature

bszwarc · 2018-11-29T16:28:20Z

docs/navigation-configuration.md

@@ -22,6 +22,7 @@ A navigation path is any existing path in the navigation tree. It connects the f

 - The path of the main application, that is, the path in the browser URL. The path is defined in a Luigi navigation node through one of the following parameters, listed in order of precedence: **externalLink**, **link**, and **pathSegment**.
 - The **viewUrl** of a micro front-end rendered in the content area of the main application.
+- If the **navCollapse** property is set to `true`, the left navigation will be hidden while user visits the particular Node.


Suggested change

- If the **navCollapse** property is set to `true`, the left navigation will be hidden while user visits the particular Node.

- If you set **navCollapse** property to `true`, the left navigation becomes hidden when you click the affected node.

I'm not sure if I didn't overthink this while wanting to make it simpler 🤔

Please add explicitly that the default value is false. And maybe 'while the user' would be better.

bszwarc

One comment to one line :)

jesusreal · 2018-11-29T16:50:08Z

core/examples/luigi-sample-angular/e2e/tests/navigation.spec.js

@@ -115,5 +115,10 @@ describe('Navigation', () => {
        expect(loc.hash).to.eq('#/projects/pr2/dps/dps1');
      });
    });
+    it('should hide left Nav', () => {
+      cy.visit('/#/projects/pr1/navCollapse');
+      cy.get('.no-left-nav');


I would be also confused with this line. Maybe you could explicitly do the check, otherwise someone could also think this line does nothing, remove it and then the test will not check what it has to check. Maybe sth like this:
cy.get('.no-left-nav').should('exist');

jesusreal · 2018-11-29T16:58:29Z

core/src/services/routing.js

-    const viewUrl = getLastNodeObject(pathData).viewUrl || '';
-    const isolateView = getLastNodeObject(pathData).isolateView || false;
+
+    const {


You get a virtual big applause for this one, awesome refactoring I really like it.

jesusreal · 2018-11-29T17:00:18Z

docs/navigation-configuration.md

@@ -22,6 +22,7 @@ A navigation path is any existing path in the navigation tree. It connects the f

 - The path of the main application, that is, the path in the browser URL. The path is defined in a Luigi navigation node through one of the following parameters, listed in order of precedence: **externalLink**, **link**, and **pathSegment**.
 - The **viewUrl** of a micro front-end rendered in the content area of the main application.
+- If the **navCollapse** property is set to `true`, the left navigation will be hidden while user visits the particular Node.


Please add explicitly that the default value is false. And maybe 'while the user' would be better.

jesusreal · 2018-11-29T17:28:18Z

core/examples/luigi-sample-angular/src/assets/extendedConfiguration.js

@@ -419,7 +425,8 @@ Luigi.setConfig({
        {
          pathSegment: 'overview',
          label: 'Overview',
-          viewUrl: '/sampleapp.html#/overview'
+          viewUrl: '/sampleapp.html#/overview',
+          navCollapse: true


It would be fine for me to leave it, but then please add some left margin to the headline in the content area

…avCollapsed-feature

bszwarc

Approved.

…e app

parostatkiem-zz added 11 commits November 28, 2018 16:00

Add navCollapse component to example app

d64b8a5

Update the example

e06b22b

Add unit test

b48478d

Prepare for the implementation

c5588e9

Merge branch 'master' of https://github.com/kyma-project/luigi into n…

5b58d6b

…avCollapsed-feature

Update docs

8b974db

Add navCollapse property to the demo node

2480d80

Improve the mechanizm using existing hideNav property

5d0396e

Remove obsolete property

a501846

Introduce 'hideLeftNav' variable

54d285b

Improve tests

4fa75ef

parostatkiem-zz added enhancement New feature or request area/luigi labels Nov 29, 2018

parostatkiem-zz requested review from antiheld, bszwarc, dariadomagala-sap, hardl, jesusreal, kazydek, klaudiagrz, kwiatekus, maxmarkus, pekura, tomekpapiernik and y-kkamil as code owners November 29, 2018 08:27

dariadomagala-sap self-assigned this Nov 29, 2018

dariadomagala-sap reviewed Nov 29, 2018

View reviewed changes

parostatkiem-zz added 3 commits November 29, 2018 10:21

Reset the unnecessary documentation changes

13466b6

Improve html formatting

aa679d6

Merge branch 'master' of https://github.com/kyma-project/luigi into n…

253c917

…avCollapsed-feature

dariadomagala-sap approved these changes Nov 29, 2018

View reviewed changes

jesusreal self-assigned this Nov 29, 2018

bszwarc reviewed Nov 29, 2018

View reviewed changes

bszwarc suggested changes Nov 29, 2018

View reviewed changes

jesusreal suggested changes Nov 29, 2018

View reviewed changes

parostatkiem-zz added 4 commits November 30, 2018 12:25

Improve the docu

66fb1eb

Add header padding

8d96365

Make e2e test more descriptive

218cfda

Merge branch 'master' of https://github.com/kyma-project/luigi into n…

dc8454b

…avCollapsed-feature

bszwarc approved these changes Nov 30, 2018

View reviewed changes

jesusreal approved these changes Nov 30, 2018

View reviewed changes

parostatkiem-zz and others added 3 commits December 3, 2018 11:52

rename 'navCollapse' to hideSideNav

6893071

Rename navCollapse to hideSideNav in documentation and angular exampl…

aba90f2

…e app

Merge branch 'master' into navCollapsed-feature

f05b7f7

jesusreal merged commit 1842d50 into SAP:master Dec 4, 2018

parostatkiem-zz deleted the navCollapsed-feature branch March 11, 2019 09:39

misappi unassigned dariadomagala-sap May 14, 2019

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

hideSideNav feature (SAP#259)

2f97bca

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nav collapsed feature #259

Nav collapsed feature #259

parostatkiem-zz commented Nov 29, 2018 •

edited

Loading

dariadomagala-sap left a comment

dariadomagala-sap Nov 29, 2018

parostatkiem-zz Nov 29, 2018

jesusreal Nov 29, 2018

dariadomagala-sap Nov 29, 2018

jesusreal Nov 29, 2018

dariadomagala-sap Nov 29, 2018

dariadomagala-sap Nov 29, 2018

bszwarc Nov 29, 2018 •

edited

Loading

jesusreal Nov 29, 2018 •

edited

Loading

bszwarc left a comment

jesusreal Nov 29, 2018

jesusreal Nov 29, 2018

jesusreal Nov 29, 2018 •

edited

Loading

jesusreal Nov 29, 2018

bszwarc left a comment

	- If the navCollapse property is set to `true`, the left navigation will be hidden while user visits the particular Node.
	- If you set navCollapse property to `true`, the left navigation becomes hidden when you click the affected node.

Nav collapsed feature #259

Nav collapsed feature #259

Conversation

parostatkiem-zz commented Nov 29, 2018 • edited Loading

dariadomagala-sap left a 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

bszwarc Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

jesusreal Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

bszwarc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusreal Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bszwarc left a comment

Choose a reason for hiding this comment

parostatkiem-zz commented Nov 29, 2018 •

edited

Loading

bszwarc Nov 29, 2018 •

edited

Loading

jesusreal Nov 29, 2018 •

edited

Loading

jesusreal Nov 29, 2018 •

edited

Loading