-
Notifications
You must be signed in to change notification settings - Fork 53
Add possibility to render submenu in MenuItem #126
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
src/components/Menu/MenuItem.tsx
Outdated
@@ -31,6 +41,9 @@ class MenuItem extends UIComponent<any, any> { | |||
/** Shorthand for primary content. */ | |||
content: customPropTypes.contentShorthand, | |||
|
|||
/** Initial submenuOpened value. */ | |||
defaultSubmenuOpened: PropTypes.bool, | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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} /> | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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, | ||
}, |
There was a problem hiding this comment.
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.
@priyankar205 and @gopalgoel19 please review this and collaborate. It seems there is some overlapping work here. |
Closing as it doesn't support our immediate needs right now |
MenuItem
This PR introduces possibility to add vertical submenu rendered in MenuItem.
TODO
API Proposal
Submenu