-
Notifications
You must be signed in to change notification settings - Fork 271
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
feat(ui5-side-navigation): initial implementation #1889
Conversation
packages/main/src/SideNavigation.hbs
Outdated
<div class="ui5-sn-bottom-content"> | ||
<div | ||
class="ui5-sn-flex-wrapper" | ||
@ui5-_click="{{handleItemClick}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would not it work with standard @ click, instead of firing ui5-_click on mouseup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good just couple of things:
-
the sub-menu items are expanded with animation in openui5
-
check the collapsed mode: the icons are not centred and the borders of the side navigation remains, when collapsed, just the content is squeezed
- the top separator of the fixed items is different on openui5, it is not 100% width, but smaller,
here how it should look like
display: inline-block; | ||
width: 250px; | ||
height: 100%; | ||
border-right: var(--sapList_BorderWidth) solid var(--sapList_GroupHeaderBorderColor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind doing RTL at a later stage, but it would be best to use effectiveDir
and set this for the left too
* | ||
* @public | ||
* @type {string} | ||
* @defaultvalue false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think it makes more sense for Fiori, not main, as it has a very specific UX (dynamic and fixed part - items/subitems), it's not just a standard container.
- When I try to interact with the sample, nothing happens (items do not expand, etc...)
- You should declare a dependency to the tree, and remove the dependency to the list in the
.js
file
Overall I think this is the right direction (using the tree) as it saves us a lot of code. I'll make a more detailed review tomorrow again, especially around the tree changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component is still in "main" package, I think we should move it to "fiori" before we merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another issue: selection is lost
Steps:
- Select a sub item, f.e. "From My Team"
- Collapse the Side Nav -> already can be seen that the selection is lost
- Expand the Side Nav -> no selection is visible
… them through tree items, rename them as _collapsed is confusing in tree context
Fixes #1871