Have optional callback in focusZoneProps to determine if scenario owner wants the input element to lose the focus on arrow key #4290
Conversation
|
|
martellaj
requested review from
ThomasMichon,
dzearing,
joschect,
jspurlin,
martellaj and
micahgodbolt
martellaj
left a comment
martellaj
left a comment
There was a problem hiding this comment.
Looks good to me. We'll be able to let consumers control which elements give and do not give up focus, without breaking functionality.
| /** | ||
| * A callback method to determine if the input element should lose focus on arrow keys | ||
| * @param {HTMLInputElement} inputElement The input element which is to loose focus. | ||
| */ |
There was a problem hiding this comment.
add default value down here.
There was a problem hiding this comment.
not sure if we could add default value to the callback method. Added @returns
| const inputValue = element.value; | ||
| const shouldInputLoseFocusOnArrowKey = this.props.shouldInputLoseFocusOnArrowKey | ||
| ? this.props.shouldInputLoseFocusOnArrowKey(element) | ||
| : false; |
There was a problem hiding this comment.
I don't love that the const shouldInputLoseFocusOnArrowKey is a boolean, but this.props.shouldInputLoseFocusOnArrowKey is a function returning a boolean.
Can we either move this function down to 822, or come up with a different name stating that this is the result of the function?
There was a problem hiding this comment.
I have addressed this comment
| (selectionStart !== inputValue.length && isForward) || | ||
| !!this.props.handleTabKey) { | ||
| (!!this.props.handleTabKey && !(this.props.shouldInputLoseFocusOnArrowKey | ||
| && this.props.shouldInputLoseFocusOnArrowKey(element))) |
There was a problem hiding this comment.
is it possible to add a unit test to cover this scenario so we don't break it in the future?
There was a problem hiding this comment.
(it's super easy, go to the packages/oufr folder, run npm run start-test, edit FocusZone.test.tsx and add a test.
There was a problem hiding this comment.
I get below error when I run start-test
Also what do you mean by packages/oufr folder
There was a problem hiding this comment.
Nevermind Joe told me what oufr means :D
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Add shouldInputLoseFocusOnArrowKey callback for scenario to determine if input should loose focus when arrow key is present when Tabbing is enabled on All elements or Input elements", |
There was a problem hiding this comment.
As a consumer I can't make sense of the reasoning here. Can you fix some of the language? This comment goes into release notes (and it can be markdown, so use backticks around property names.)
FocusZone: adding shouldInputLoseFocusOnArrowKey optional callback for scenarios where the user presses an arrow key on an input element, but we would want to disable moving focus away.
That's just an idea but maybe you could help refine the language.
There was a problem hiding this comment.
Oh good to know. Thanks David. I have updated the change log.
5 participants
Description of changes
If the focus zone has specified HandleTabKey = FocusZoneTabbableElements.All then arrowing up and down inside the search box input element does not work (which is expected), But SearchBox can be part of other components such as Context Menu. In this case its very natural for user to press up and down to navigate between the menu items one of them being . Rather than defaulting the behavior for all the scenarios, I am adding an optional callback called as shouldInputLoseFocusOnArrowKey that will be called if it is defined inside function _shouldInputLoseFocus in . If its not specified nothing changes. But if its specified we call it when we have a support for Tabbing specified to a none (undefined) value.