feat(menuItem: add menu prop)

[gopalgoel19]

menuItem menu prop

Add a menu prop which has the capability to render submenus.

TODO

• Conformance test
• Minimal doc site example
• Stardust base theme
• Teams Light theme
• Teams Dark theme
• Teams Contrast theme
• Confirm RTL usage
• W3 accessibility check
• Stardust accessibility check
• Update glossary props table
• Update the CHANGELOG.md

[codecov]

Codecov Report

Merging #357 into master will decrease coverage by 0.19%.
The diff coverage is 72.72%.

@@            Coverage Diff            @@
##           master     #357     +/-   ##
=========================================
- Coverage   89.75%   89.56%   -0.2%     
=========================================
  Files          64       64             
  Lines        1259     1274     +15     
  Branches      159      189     +30     
=========================================
+ Hits         1130     1141     +11     
- Misses        126      129      +3     
- Partials        3        4      +1

Impacted Files	Coverage Δ
src/components/Menu/MenuItem.tsx	76.74% <64.28%> (-3.26%)	⬇️
src/components/Menu/Menu.tsx	97.22% <87.5%> (-2.78%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac6a25...35e8a76. Read the comment docs.

[gopalgoel19]

Can someone investigate why using this syntax is creating trouble?

[kuzhelov]

which trouble it creates, specifically?

Please, note that when you are using

content={<Menu items={menu.items} type={type} vertical/>}

you don't introduce any wrapper for the Popup's content - and it could happen that the content that you are providing unwrapped (by the expression mentioned above) could have z-index value that would prevent it to be shown on top of the page's content. Consider to review this example for the Popup for more details: https://stardust-ui.github.io/react/components/popup#types-popup-content-wrapper-example

[gopalgoel19]

Ah, totally missed the z-index. Thanks for pointing it out.

[gopalgoel19]

When should we use setState vs. trySetState? Can someone please explain in brief the purpose of trySetState.

[kuzhelov]

lets schedule a short sync for tomorrow, I will explain the concept to you. Before that, please, consider to read about Controlled and Uncontrolled Components, as well as try to make experiments with input component being used in these different modes - note how the value updates for each, when onChange handler is triggered, how value updates in controlled mode.

• https://reactjs.org/docs/uncontrolled-components.html
• https://reactjs.org/docs/forms.html

[jurokapsiar]

From ARIA point of view, menu and submenu needs to conform to menu design pattern for both DOM structure and keyboard handling. Please go through the design pattern to understand the requirements. This design pattern is done as a dropdown - I suppose you can achieve similar functionality with popups, but that would require testing. Focus trap zone (#239) should help you with focusing the items on open. But in general, you should look at #126 which implements submenu. Was there a reason why you were not able to follow this approach?

[gopalgoel19]

@jurokapsiar I did look at #126 before opening this PR. I went ahead with Popup as it provides alignment and positioning capabilities out of the box. Let me explore what Focus trap zone offers. I'll try to fix accessibility next.
Also, if you strongly suggest to implement it as done in #126, let me know. I will then move ahead in that direction.

[jurokapsiar]

@gopalgoel19 it is hard to say before all requirements are fully implemented - keyboard navigation is the first step, then once feature complete, we would need to test with screen readers. For positioning, I believe you can use popper.js and computePopupPlacement utility function the same way how popup uses it. Based on previous experience, following ARIA patterns saves us a lot of time. For #126 we even did validation, which you would need to do from scratch when using popup.

[levithomason]

Always destructure state and pros, see line 150:

     const { activeIndex } = this.state

Here you would add the keys you want to use. activeIndex is already destructured in scope, so you can use it directly. submenuOpen needs to be added to the destructured values. Destructuring keeps code clean, but more importantly it also breaks object references.

[levithomason]

• Use nested ul instead of Popup, popup is placed at the root of the DOM opposed to the correct nested location
• Add icons indicating submenu
• Submenu needs to focus on open
• Keyboard navigation for submenu

The requirements and examples for these issues can be found in @jurokapsiar's links above.