[WIP] feat(ContextMenu): add base implementation #340
Conversation
c35d235 to
549a450
Compare
Codecov Report
@@ Coverage Diff @@
## master #340 +/- ##
==========================================
- Coverage 89.59% 89.54% -0.05%
==========================================
Files 62 64 +2
Lines 1220 1243 +23
Branches 156 181 +25
==========================================
+ Hits 1093 1113 +20
- Misses 125 128 +3
Partials 2 2
Continue to review full report at Codecov.
|
|
please look at the submenu PR which implements an accessible submenu: #126 |
| itemProps.onClick = onItemClick | ||
| return ListItem.create(item, { defaultProps: itemProps }) | ||
| }) | ||
| return <List selection={true}>{children}</List> |
There was a problem hiding this comment.
use Menu component for API consistency, styling and accessibility. If Menu does not support all requirements needed, we can extend it.
There was a problem hiding this comment.
@jurokapsiar That is the reason I used Menu component when I started building it. However, I moved away from Menu to List because List component fulfilled all the requirements needed( was not sure about accessibility ). But alright, let me stick to Menu component.
| }} | ||
| // content={<ContextMenu items={item.menu.items} onItemClick={onItemClick} />} | ||
| > | ||
| {ListItem.create(item, { defaultProps: itemProps })} |
There was a problem hiding this comment.
use MenuItem component for API consistency, styling and accessibility. If MenuItem does not support all requirements needed, we can extend it.
| /** The tree of menu containing submenus and it's submenus and so on. */ | ||
| items: PropTypes.arrayOf(PropTypes.object), | ||
|
|
||
| /** Function passed which needs to be invoked when menuitem has no submenu */ |
There was a problem hiding this comment.
Don't really understand comment. For menu, we have onclick on the menu-items. Can't we do same here? It's very likely that different menu items what to call different functions.... (and nit - end comment with '.')
There was a problem hiding this comment.
Yes, but this just handles the case of one callback function for all menu-items. The onClick on all items will also work at the same time.
| fontFamily: 'Segoe UI', | ||
| ...(item.divider && { | ||
| borderBottom: '2px solid #F3F2F1', | ||
| }), |
|
As per the discussion with Levi, it would be better to build a ContextMenu form scratch instead of relying on Menu/List component. I will be working on a new PR and closing this PR for now. |
ContextMenu
TODO