Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

Add possibility to render submenu in MenuItem #126

Closed
wants to merge 8 commits into from

Conversation

sophieH29
Copy link
Contributor

@sophieH29 sophieH29 commented Aug 23, 2018

MenuItem

This PR introduces possibility to add vertical submenu rendered in MenuItem.

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

API Proposal

Submenu

submenu

const submenuData = {
     items: [
           { key: 'new', content: 'New' },
           { key: 'open', content: 'Open' },
     ]
}
<MenuItem submenu={submenuData} content="File" />
<li>
  <a> File </a>
  <ul>
    <li> <a> New </a> </a>
    <li> <a> Open </a> </a>
 </ul>
</li>

@sophieH29
Copy link
Contributor Author

sophieH29 commented Aug 23, 2018

Having a concern regarding style:

Menu and menu items are having a transparent background. So when a submenu is rendered, it can look weird, as everything behind it is visible.

Looking forward to your thoughts and ideas!

@codecov
Copy link

codecov bot commented Aug 23, 2018

Codecov Report

Merging #126 into master will increase coverage by 0.15%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   67.82%   67.97%   +0.15%     
==========================================
  Files         101      101              
  Lines        1386     1396      +10     
  Branches      275      269       -6     
==========================================
+ Hits          940      949       +9     
- Misses        443      444       +1     
  Partials        3        3
Impacted Files Coverage Δ
src/themes/teams/components/Menu/menuVariables.ts 50% <ø> (ø) ⬆️
src/themes/teams/components/Menu/menuStyles.ts 20% <0%> (ø) ⬆️
src/themes/teams/components/Menu/menuItemStyles.ts 7.79% <0%> (-0.11%) ⬇️
src/components/Menu/MenuItem.tsx 94.11% <100%> (+1.8%) ⬆️
src/components/Menu/Menu.tsx 100% <100%> (ø) ⬆️

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 57eb518...1c6ec90. Read the comment docs.

@kuzhelov kuzhelov added the RFC label Aug 23, 2018
@@ -31,6 +41,9 @@ class MenuItem extends UIComponent<any, any> {
/** Shorthand for primary content. */
content: customPropTypes.contentShorthand,

/** Initial submenuOpened value. */
defaultSubmenuOpened: PropTypes.bool,

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment will be used in the docs page for describing the property. Please use more descriptive comment avoiding any properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mnajdova that makes sense. But I've made the same description as for defaultActiveindex prop in Accordion and Menu components to keep consistency. Should I change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah, alright then, leave it as is.

@notandrew
Copy link
Contributor

I'd expect the submenu to have the same color as the menu bar. Is it possible to apply this style?


const MenuExampleWithSubmenu = () => (
<Menu defaultActiveIndex={0} items={items} variables={menuWidthVariable} />
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to add some height on the element as the content is not visible (try to wrap the Menu in some wrapper with some bigger height).
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I would expect to be able to expand only one submenu at a time. I don't think the scenario above (having three different dropdowns should work (try closing the submenu if there is click outside of it).

position: 'absolute',
borderTopLeftRadius: 0,
borderTopRightRadius: 0,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally support @notandrew comment, the type should affect the submenu as well.

@levithomason
Copy link
Member

@priyankar205 and @gopalgoel19 please review this and collaborate. It seems there is some overlapping work here.

@sophieH29
Copy link
Contributor Author

sophieH29 commented Oct 5, 2018

Closing as it doesn't support our immediate needs right now

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants