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-menu): Initial implementation #4742

Merged
merged 32 commits into from
Apr 14, 2022
Merged

feat(ui5-menu): Initial implementation #4742

merged 32 commits into from
Apr 14, 2022

Conversation

NHristov-sap
Copy link
Contributor

@NHristov-sap NHristov-sap commented Feb 14, 2022

This is the initial implementation of and web components that allow creating of a hierarchical menu structure.

// Menu API
       properties:  {
		headerText: {
			type: String,
		},
	slots: {
		         "default": {
			   propertyName: "items",
			   type: HTMLElement,
			    invalidateOnChildChange: true,
		},
	},
	events:
		 "item-click": {
			detail: {
				item: {
					type: Object,
				},
				text: {
					type: String,
				},
			},
		 },
	}
	
	
// MenuItem API
	properties: {
		text: {
			type: String,
		},
		icon: {
			type: String,
		},
		startsSection: {
			type: Boolean,
		},
		disabled: {
			type: Boolean,
		}
	}

}

_createSubMenu(item, openerId) {
const subMenu = document.createElement("ui5-menu");
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole method is concerning - at first glance it seems that what you need is a static area template, where this sub menu addition is going to happen. You cannot append to the body as it belongs to the app. For all such purposes you must use the static area. Also, you cannot use document.createElement, you must use the .hbs file for creating DOM. Finally, hard-coded ui5-menu is not scoping-friendly - if the user set a scoping suffix such as test, it would be f.e. ui5-menu-test. When you use the .hbs, the compiler will do this automatically. However, this component creates other instances of itself, so I'm not even sure this is going to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it can be done via hbs template. In fact, a new ui5-menu element must be created for each nested submenu.

@vladitasev
Copy link
Contributor

I pushed a small update to address some of the issues:

  • created the missing MenuItem.css file that broke the build (it's empty)
  • added a div.submenus in the shadow root
  • made the _createSubmenus function use this div instead of the body
  • used a scoping-friendly API to get the tag of the component

Please let me know if everything works with the proposed changes

packages/main/src/ListItem.js Outdated Show resolved Hide resolved
packages/main/src/ListItem.js Outdated Show resolved Hide resolved
Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

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

Can we add someone from List team and an UA colleague for the documentation part?

packages/main/src/ListItem.js Outdated Show resolved Hide resolved
packages/main/src/ListItem.js Show resolved Hide resolved
packages/main/src/Menu.hbs Outdated Show resolved Hide resolved
packages/main/src/Menu.js Show resolved Hide resolved
packages/main/src/Menu.js Show resolved Hide resolved
packages/main/src/Menu.js Show resolved Hide resolved
packages/main/src/MenuItem.js Show resolved Hide resolved
packages/main/src/Menu.hbs Show resolved Hide resolved
packages/main/src/Menu.js Outdated Show resolved Hide resolved
packages/main/src/Menu.js Outdated Show resolved Hide resolved
@ilhan007 ilhan007 requested a review from dobrinyonkov April 4, 2022 07:52
packages/main/src/Menu.js Outdated Show resolved Hide resolved
packages/main/src/Menu.js Outdated Show resolved Hide resolved
* @public
* @returns {object} the item that opened the sub menu containing the current item.
*/
parent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not to provide such a function for the time being. First, I'm not sure about the name: if it is called just parent it should probably be a getter, not a method, similarly to how HMLT's native parentElement and parentNode work. Also, I'm not sure why people couldn't use parentElement recursively to traverse the HTML they created themselves. Finally, I think the app has other ways to get such information, if needed - for example if they have two "Save" manu items in different subtrees, they can give them different IDs if they need to know which one in particular was selected by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

I found out one bug with the keyboard navigation, when I tried to get back from subitem, using the ArrowLeft key - the focus is lost and the user can't continue using the keyboard:

  1. Open sample with sub items
  2. Navigate to the 3rd level item with ArrowRight
  3. Then, try to go back with ArrowLeft
Screen.Recording.2022-04-14.at.09.43.18.mov

@ilhan007
Copy link
Member

it seems it's reproduces only on my side, so I will get back my comment and approve the change

@ilhan007 ilhan007 merged commit deac309 into master Apr 14, 2022
@ilhan007 ilhan007 deleted the menu_menuitem_initial branch April 14, 2022 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants