Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display info in bottom left corner #503

Merged

Conversation

parostatkiem-zz
Copy link
Contributor

@parostatkiem-zz parostatkiem-zz commented Apr 30, 2019

Description

Changes proposed in this pull request:

  • Display a configurable string at the bottom of LeftNav
  • It follows the design and is sticky
  • Add e2e test for the new feature
  • Update docu of the new luigi-config setting

Related issue(s)

@maxmarkus maxmarkus self-assigned this Apr 30, 2019
@maxmarkus maxmarkus added this to the Sprint_Swinka_13 milestone Apr 30, 2019
Copy link
Contributor

@maxmarkus maxmarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my comments.

@@ -20,3 +21,4 @@ settings: {
* **header.title** defines the top left navigation title.
* **header.favicon** defines the favicon. It rquires a standard favicon file with the `.ico` extension, and 16x16px or 32x32px dimensions.
* **responsiveNavigation** allows adding a button on the left side of the top navigation. Upon click, the button shows or hides the left navigation. The possible values are `simple` and `simpleMobileOnly`. `simple` displays the button regardless of the browser window´s size, while `simpleMobileOnly` shows the button when the width is lower than `600px`. If you don't specify any value for **responsiveNavigation**, the button will not be displayed. The same applies when you enable **hideSideNav** for the current active navigation node.
* **sideNavFooterText** a string that is displayed in a sticky footer inside Side Navigation. Good place to display the version of your application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also be a function (not async) since we are using getConfigValue to fetch it. Do we want to mention this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, it can't be. Checked with arrow and normal function.
image

@@ -291,4 +305,12 @@
width: 1.25rem;
}
}

.lui-sideNavFooter {
position: sticky;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed instead of sticky? It seems to be placed not at the end of the navbar.
screen markus e 2019-04-30 at 15 10 07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the conversation on Slack about this, I understood it as sticky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but looking at the Zeplin, it's fixed. I'm confused 😕

@@ -139,6 +139,13 @@
{/if} {/each}
</nav>
{/if}
{#if isFooterVisible(footerText)}
<div class="lui-sideNavFooter fd-has-padding-small fd-has-type-minus-1">
{#if footerText}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the container at all, if footerText is undefined? else I would put it before isFooterVisible.

Copy link
Contributor Author

@parostatkiem-zz parostatkiem-zz Apr 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did the implementation (this template + isFooterVisible function) with having the future placement of SideNav close button on mind. It's visible on Zeplin project. Lydia says it's a concept and likely be introduced in the future.
image

Furthermore, who knows - maybe we'll put there something else in the future?
In general, I'm against useless containers (!) but this is a special case in my opinion.

@marynaKhromova marynaKhromova self-assigned this May 2, 2019
Copy link
Contributor

@marynaKhromova marynaKhromova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach you found for this task 👍

I left some suggestions. Let me know if you have any questions, we can discuss it :)

core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
core/src/navigation/LeftNav.html Outdated Show resolved Hide resolved
Co-Authored-By: Klaudia Grzondziel <[email protected]>
@parostatkiem-zz parostatkiem-zz merged commit ac04bfd into SAP:master May 13, 2019
@parostatkiem-zz parostatkiem-zz deleted the Display-info-in-bottom-left-corner branch May 13, 2019 06:46
stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021
* Add sideNav footer with static text

* Render the footer conditionaly & render the configurable text

* add a new setting to luigi-config

* update docu

* Add e2e test

* Add sticky footer behaviour

* Display LuigiClient version in Angular example app

* Update docs/general-settings.md

Co-Authored-By: Klaudia Grzondziel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants