Add support for keyboard navigation #73
Conversation
| @@ -352,10 +352,14 @@ const Menu = (props) => { | |||
| props.selectProps.listProps.onScroll(e.target); | |||
There was a problem hiding this comment.
@ledsoft It does not seem to be used anywhere, are there scenarios where it is needed? It seems like the component works better without this custom Menu component
There was a problem hiding this comment.
Could you be more specific as to what you mean by "works better"?
There was a problem hiding this comment.
Without custom Menu, expected behavior is working as intended (i.e. keyboard navigation enabled, no scroll to top when clicking). With the custom Menu, I had to implement onMouseEnter to enable keyboard navigation (does not work with onMouseDown)
There was a problem hiding this comment.
Ok, in that case why don't you go ahead and remove the custom Menu component altogether. It was added there just to fix some scrolling issues, maybe that is no longer necessary.
There was a problem hiding this comment.
Ok, I was just concerned it is here for other cases I'm not aware of but yes then I'll remove it.
| @@ -352,10 +352,14 @@ const Menu = (props) => { | |||
| props.selectProps.listProps.onScroll(e.target); | |||
There was a problem hiding this comment.
Could you be more specific as to what you mean by "works better"?
| @@ -352,10 +352,14 @@ const Menu = (props) => { | |||
| props.selectProps.listProps.onScroll(e.target); | |||
| }, | |||
| //Enables option selection even when input is not focused | |||
| onMouseDown: (event) => { | |||
| onMouseEnter: (event) => { | |||
There was a problem hiding this comment.
Isn't this a bit too eager? Mouse enter will trigger focus whenever mouse enters the menu list, which could mean it will steal focus from other components accidentally. How does this help with the keyboard navigation?
There was a problem hiding this comment.
Yes I agree that it is quite eager, if you think it makes sense I would remove this custom Menu (see reply above), otherwise I will think about another implementation.
There was a problem hiding this comment.
Let's remove the component in that case.
|
@LaChope Could you check that your code is synchronized with the base? I see conflicts in |
|
So we should revert #71? Will the component work in s-forms with React 18 without it? That was, I believe, the reason the PR was originally created... |
|
Yes it can be reverted, in the end this issue was solved with a simpler fix here: kbss-cvut/s-forms#232 (using intelligent-tree |
|
@LaChope Ok, merged and published as 0.11.2. |
@ledsoft This PR aims to solve kbss-cvut/s-forms#258