feat: add divider & group label to menu & menu list, #198

[SarahHouben]

fixes: #198

TSIA

[diondiondion]

Thanks @SarahHouben, this is looking good so far! I noticed a few issues, see comments.

It'd also be great if you could give the Menu component a test run using a screen reader to see if the new group headers are announced properly. (It's not necessarily a bug if they don't, as menu groups seem slightly non-standard and aren't mentioned in the WAI-ARIA practices.)

[SarahHouben]

Thanks @SarahHouben, this is looking good so far! I noticed a few issues, see comments.

It'd also be great if you could give the Menu component a test run using a screen reader to see if the new group headers are announced properly. (It's not necessarily a bug if they don't, as menu groups seem slightly non-standard and aren't mentioned in the WAI-ARIA practices.)

So, when it comes to announcing of the group headers, the screen reader sometimes does it (see screenshot) but most of the times it skips them. Not really sure why it's behaving that way tbh. Most of the time it just announced "label, menuitem, group"

[diondiondion]

There's just one more issue around the default element used for the MenuList components – sorry if my previous comment about that wasn't super clear. Otherwise this looks great, thank you!

[diondiondion]

This isn't necessary here, menuListProps already contains the role:

base5-ui/src/Menu/Menu.js

Lines 114 to 116 in 8d5949a

	const menuListProps = {
	ref: menuListRef,
	role: 'menu',

But it would be great if you could add as="div" here, and revert the changes made to the styled.x definitions made in MenuList/index.js. Those should still default to ul/li as before (for example for the NavMenu component which doesn't add any roles), and should only be overridden when a role is provided.

Suggested change

	<MenuListUI.Wrapper role="menu" {...menuListProps}>
	<MenuListUI.Wrapper as="div" {...menuListProps}>

The same would have to be done for the MenuItem component: https://github.com/5app/base5-ui/blob/master/src/Menu/MenuItem.js#L22

[SarahHouben]

Ah okay, sorry, I completely misunderstood earlier. 😬

[5app-Machine]

🎉 This PR is included in version 14.11.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀