[Menu] Add onFocusIndexChange property #5851
Conversation
Add a callback property that is called whenever the MenuItem focus is changed. Sometimes it will be called when the focus isn't changed. For example: * If top menu item is keyboard focused (call mui#1) and then the up arrow is pressed (call mui#2). * If some menu item is keyboard focused (call mui#1) and then that same menu item is clicked (call mui#2).
muibot
added
PR: Needs Review
PR: needs revision
|
Hmmm... I used Looking into it. |
Unmounting the Menu component in order to cancel the FocusRipple.pulsate() timer. Before returning, this timer callback always starts a new setTimeout() for itself. This outstanding timer stopped Node.js from exiting.
| let index = this.state.focusIndex; | ||
| const maxIndex = this.getMenuItemCount(filteredChildren) - 1; | ||
|
|
||
| index++; | ||
| if (index > maxIndex) index = maxIndex; | ||
|
|
||
| this.setFocusIndex(index, true); | ||
| this.setFocusIndex(index, true, event); |
There was a problem hiding this comment.
We use the event as the last and first argument. I think that it should always been the first argument.
Could we change that here?
|
|
||
| assert(onMenuItemFocusChangeSpy.calledWith(null, 0), | ||
| 'initial focus should invoke callback with 0'); | ||
| onMenuItemFocusChangeSpy.reset(); |
There was a problem hiding this comment.
Why not using the beforeEach / afterEach pattern to better isolate the tests?
There was a problem hiding this comment.
I am very interested in making sure we don't break what happens when the user enters a particular sequence of arrow key events. That would be difficult for me to test if I isolated each arrow key event inside its own it since I would lose all the Menu state along with the spy state.
Does that make sense? I'm not confident I interpreted your comment correctly.
Put the `event` argument at the front to be consistent with other functions.
Add a spec for selecting a MenuItem via hotkeys. The new spec uncovered two pre-existing bugs.
|
Thanks for your feedback! I just made your suggested change to the order of the I also just added a unit test for some code that I realized I hadn't even tested manually. In the process, I noticed one or two pre-existing bugs in the hotkey code. They are documented in the tests. Do you think I should open issues for them? |
oliviertassinari
added
PR: Review Accepted
and removed
PR: Needs Review
PR: needs revision
There was a problem hiding this comment.
I noticed one or two pre-existing bugs in the hotkey code. They are documented in the tests. Do you think I should open issues for them?
Sounds like to issues indeed. You can open some issue. But I'm not sure someone will solve them as we are working on the next branch.
|
Thanks! |
zannager
added
the
component: menu
Note
This PR is a follow-up to issue #5790, where I described the use case that I am trying to solve.
Description
Add a callback property that is called whenever the MenuItem focus is changed.
Sometimes it will be called when the focus isn't changed. For example: