Fiori3 compliant left navigation collapse #624
Conversation
marynaKhromova
requested review from
hardl,
jesusreal,
JohannesDoberer,
maxmarkus and
pekura
as code owners
maxmarkus
changed the title
core/src/App.html
Outdated
| $topNavHeight: 48px; | ||
| $leftNavWidth: 320px; | ||
| $leftNavWidthCollapsed: 40px; | ||
| $desktop-min-width: 600px; |
There was a problem hiding this comment.
I know you didn't write it, but please change $desktop-min-width to be also camelCased.
| @@ -781,8 +783,12 @@ | |||
| </script> | |||
|
|
|||
| <style type="text/scss"> | |||
| /*Mixins*/ | |||
There was a problem hiding this comment.
Guess that comment is not required.
core/examples/luigi-sample-angular/src/luigi-config/extended/settings.js
Outdated
Show resolved
Hide resolved
| > | ||
| {#if isOpenUIiconName(nodes.metaInfo.icon)} | ||
| <span | ||
| class="fd-side-nav__icon {'sap-icon--' + nodes.metaInfo.icon} {isSemiCollapsed && !nodes.metaInfo.icon ? 'sap-icon--rhombus-milestone-2' : ''} sap-icon--l" |
There was a problem hiding this comment.
Would be cool to add alt attribute of {key} to that span and img tags, so one can hover over it in collapsed mode to see what is written if he does not recognize the icon? @hardl do you agree?
There was a problem hiding this comment.
good idea 👍 done.
core/src/navigation/LeftNav.html
Outdated
| document | ||
| .getElementsByClassName('fd-side-nav')[0] | ||
| .setAttribute('style', 'overflow-y:auto;'); | ||
| }, 50); |
There was a problem hiding this comment.
Why 50? If you want to have in the next tick, you can just leave the number empty.
| selectedCategory = nodeOrNodes.metaInfo.label; | ||
| } else { | ||
| selectedCategory = | ||
| (nodeOrNodes.category && nodeOrNodes.category.label) || |
There was a problem hiding this comment.
Due to the babel plugin, we can use now (nodeOrNodes.category?.label)
There was a problem hiding this comment.
I had to revert it because integration tests were failing with this change
| let selectedCategory; | ||
| let sideBar = document.getElementsByClassName('fd-app__sidebar')[0]; | ||
|
|
||
| if (nodeOrNodes.metaInfo && nodeOrNodes.metaInfo.label) { |
There was a problem hiding this comment.
Due to the babel plugin, we can use now (nodeOrNodes.metaInfo?.label).
There was a problem hiding this comment.
I had to revert it because integration tests were failing with this change
core/src/navigation/LeftNav.html
Outdated
| //Calculate top/bottom position for flyout sublist | ||
| let parent = el.closest('.fd-side-nav__group'); | ||
| let parentTopPosition = parent.offsetTop; | ||
| let shellbarHeight = Luigi.elements().getShellbar().offsetHeight; |
There was a problem hiding this comment.
Internally we should not rely on the window.Luigi object, since you already added import { LuigiElements } from '../core-api'; you can use that one.
There was a problem hiding this comment.
good point! done)
| }); | ||
|
|
||
| StateHelpers.doOnStoreChange(this.store, () => { | ||
| this.set({ | ||
| footerText: LuigiConfig.getConfigValue('settings.sideNavFooterText') | ||
| }); | ||
| }); | ||
|
|
||
| // set isSemiCollapsed to true for mobile |
There was a problem hiding this comment.
Check with @hardl if the responsiveNav setting should be able to be changed during runtime. If yes, the following code should be moved into the onStoreChange block.
There was a problem hiding this comment.
@hardl said he sees no usecase where you would change that on runtime.
…ettings.js Co-Authored-By: Markus <1720843+maxmarkus@users.noreply.github.com>
…of github.com:marynakhromova/luigi into 453-Fiori3-compliant-left-navigation-collapse-feature
…nts, remove delay on setTimeOut
Co-Authored-By: Markus <1720843+maxmarkus@users.noreply.github.com>
|
good job!! |
marynaKhromova
deleted the
453-Fiori3-compliant-left-navigation-collapse-feature
branch
Description
Changes proposed in this pull request:
fd-app__sidebarandlui-side-nav__footerbased on flexboxsap-icon--lui-blankas it's not been usedHow to test it
luigi-sample-angular/src/luigi-config/extended/settings.jsand setsemiCollapsibleforresponsiveNavigationcollapsible: trueto some nodes inluigi-sample-angular/src/luigi-config/extended/projectDetailNav.js