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

feat(ui5-side-navigation): initial implementation #1889

Merged
merged 31 commits into from
Jul 23, 2020
Merged

Conversation

fifoosid
Copy link
Contributor

@fifoosid fifoosid commented Jun 30, 2020

  • Tests are added

image

Fixes #1871

<div class="ui5-sn-bottom-content">
<div
class="ui5-sn-flex-wrapper"
@ui5-_click="{{handleItemClick}}"
Copy link
Member

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

Copy link
Member

@ilhan007 ilhan007 left a 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

now:
Screenshot 2020-07-02 at 11 50 59 AM

openui5
Screenshot 2020-07-02 at 11 54 53 AM

  • the top separator of the fixed items is different on openui5, it is not 100% width, but smaller,
    here how it should look like

Screenshot 2020-07-02 at 11 53 40 AM

packages/main/src/SideNavigation.js Outdated Show resolved Hide resolved
packages/main/src/SideNavigation.js Outdated Show resolved Hide resolved
packages/main/src/SideNavigation.js Outdated Show resolved Hide resolved
packages/main/src/SideNavigation.js Outdated Show resolved Hide resolved
packages/main/src/SideNavigationItem.hbs Outdated Show resolved Hide resolved
packages/main/src/themes/Icon.css Outdated Show resolved Hide resolved
packages/main/src/themes/SideNavigation.css Outdated Show resolved Hide resolved
display: inline-block;
width: 250px;
height: 100%;
border-right: var(--sapList_BorderWidth) solid var(--sapList_GroupHeaderBorderColor);
Copy link
Contributor

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

packages/main/src/themes/SideNavigationItem.css Outdated Show resolved Hide resolved
packages/main/test/pages/SideNavigation.html Outdated Show resolved Hide resolved
@vladitasev
Copy link
Contributor

vladitasev commented Jul 2, 2020

Some high level comments:

  • Let's keep it to two levels only (like in OpenUI5), but have separate items: side navigation item and side navigation subitem. The subitem won't have the slot and the icon.
  • You can try to use the list internally with the TreeListItem, or why not - the whole ui5-tree. The goal is to have the proper design and acc without much overhead. This goes for the special left/right keyboard handling for opening/closing tree nodes.
  • When collapsed, and the popup appears, the popup should not be centered around the selected item, but rather should be on the same level:
    image
  • The user should never have to set width. Width should be set naturally by the items. Furthermore, width is predefined when collapsed, and if the user set width before that, it would be an issue.
  • The user should rarely have to set height: if they do - it would be commonly 100% to make it cover the whole page.

*
* @public
* @type {string}
* @defaultvalue false
Copy link
Member

Choose a reason for hiding this comment

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

the default is wrong

Copy link
Contributor

@vladitasev vladitasev left a 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.

packages/main/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/main/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/main/src/SideNavigationItem.js Outdated Show resolved Hide resolved
packages/main/src/TreeItem.js Outdated Show resolved Hide resolved
Copy link
Member

@ilhan007 ilhan007 left a 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.

Copy link
Member

@ilhan007 ilhan007 left a 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:

  1. Select a sub item, f.e. "From My Team"
  2. Collapse the Side Nav -> already can be seen that the selection is lost
  3. Expand the Side Nav -> no selection is visible

packages/fiori/src/SideNavigation.js Outdated Show resolved Hide resolved
packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
@vladitasev vladitasev dismissed ilhan007’s stale review July 23, 2020 11:42

Comments addressed

@fifoosid fifoosid merged commit 47b38cc into master Jul 23, 2020
@fifoosid fifoosid deleted the side-navigation branch July 23, 2020 11:52
@ilhan007 ilhan007 added this to 1.0.0-rc.8 (released) in UI5 Web Components - Roadmap Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a SideNavigation component
3 participants