Bugfix/iterate over selectable elements #294
Conversation
✅ Deploy Preview for firefox-devtools-react-contextmenu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| } | ||
|
|
||
| if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0) { | ||
| if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0 && child.props.children instanceof Object) { |
There was a problem hiding this comment.
for a menu that looks like the test below:
<ContextMenu id={data.id} onHide={jest.fn()}>
<MenuItem onClick={jest.fn()}>Item 1</MenuItem>
<MenuItem onClick={jest.fn()}>Item 2</MenuItem>
<div>custom-content</div>
</ContextMenu>what happens then? Is the div part of the collected children? I feel like it shouldn't, but maybe I'm wrong (as it could be good for accessibility).
What do you think?
There was a problem hiding this comment.
yes, the div becomes a part of the collected children.
If the library allows adding custom elements and displays them in the menu, then I believe it should allow selecting them with the keyboard as well.
There was a problem hiding this comment.
I see; I was thinking that it would be more like headers to separate groups, and then we wouldn't want to select them...
but I don't know really.
I think @canova experimented a bit with adding custom content in the profiler UI's track button. @canova would you have an opinion about this? On my side I don't mind landing this if you agree too!
There was a problem hiding this comment.
Yeah, if I remember correctly I wasn't very happy with the way we collect children in the menu. But I think it's fine to collect custom content there. And this PR is mostly making sure that we don't throw an error, so I think this PR is fine.
| } | ||
|
|
||
| if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0) { | ||
| if ([MenuItem, this.getSubMenuType()].indexOf(child.type) < 0 && child.props.children instanceof Object) { |
There was a problem hiding this comment.
Yeah, if I remember correctly I wasn't very happy with the way we collect children in the menu. But I think it's fine to collect custom content there. And this PR is mostly making sure that we don't throw an error, so I think this PR is fine.
The library allows to use custom html elements next to menu items, but throws an error when using the keyboard to navigate.
To reproduce the issue, remove the fix and run the unit test
should allow keyboard actions when menu contains a non menu item element.The fix does not introduce any new functionality, but rather makes the current implementation more explicit and fail-proof.