Migrate ArrowKeyFocusManager to functional component #19812
Conversation
melvin-bot
bot
requested review from
fedirjh and
youssef-lr
and removed request for
a team
|
@fedirjh @youssef-lr One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| }); | ||
|
|
||
| const onArrowUpKey = useCallback(() => onArrowUpKeyRef.current(), []); | ||
| const onArrowDownKey = useCallback(() => onArrowDownKeyRef.current(), []); |
There was a problem hiding this comment.
The way we set the key handlers is a bit weird, but I think it is necessary here, if we want to avoid unnecessary re-renders or resubscribing to the KeyboardShortcut inside useEffect below.
I am trying to accomplish something akin to the useEvent hook from this RFC
|
cc @marcaaron @youssef-lr @aldo-expensify @szebniok Given that we have created custom hooks useArrowKeyFocusManager.js and useKeyboardShortcut.js. I believe we should utilize those hooks in this migration. However, there is an open issue regarding the Arrow key shortcut not working, which affects the same hooks. Should we hold the PR until the issue is resolved ? @szebniok, could you please take a look at the issue #19700 and assess if we can address it ? |
| useEffect(() => { | ||
| onArrowUpKeyRef.current = () => { | ||
| if (props.maxIndex < 0) { | ||
| return; | ||
| } | ||
|
|
||
| const currentFocusedIndex = props.focusedIndex > 0 ? props.focusedIndex - 1 : props.maxIndex; | ||
| let newFocusedIndex = currentFocusedIndex; | ||
|
|
||
| while (props.disabledIndexes.includes(newFocusedIndex)) { | ||
| newFocusedIndex = newFocusedIndex > 0 ? newFocusedIndex - 1 : props.maxIndex; | ||
| if (newFocusedIndex === currentFocusedIndex) { | ||
| // all indexes are disabled | ||
| return; // no-op | ||
| } | ||
| } | ||
| props.onFocusedIndexChanged(newFocusedIndex); | ||
| }; | ||
|
|
||
| onArrowDownKeyRef.current = () => { | ||
| if (props.maxIndex < 0) { | ||
| return; | ||
| } | ||
|
|
||
| const currentFocusedIndex = props.focusedIndex < props.maxIndex ? props.focusedIndex + 1 : 0; | ||
| let newFocusedIndex = currentFocusedIndex; | ||
|
|
||
| while (props.disabledIndexes.includes(newFocusedIndex)) { | ||
| newFocusedIndex = newFocusedIndex < props.maxIndex ? newFocusedIndex + 1 : 0; | ||
| if (newFocusedIndex === currentFocusedIndex) { | ||
| // all indexes are disabled | ||
| return; // no-op | ||
| } | ||
| } | ||
| props.onFocusedIndexChanged(newFocusedIndex); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
I don't understand the purpose of this code in a useEffect that will be run in every cycle after rendering. Why did we put it in a use effect and use references (useRef)?
| const onArrowUpKey = useCallback(() => onArrowUpKeyRef.current(), []); | ||
| const onArrowDownKey = useCallback(() => onArrowDownKeyRef.current(), []); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
NAB: I think this would be a bit simpler with each subscribe in its own useEffect.
| useEffect(() => { | ||
| if (mounted.current) { | ||
| if (prevMaxIndex.current === props.maxIndex) { | ||
| return; |
There was a problem hiding this comment.
This useEffect is not watching anything either, is that intended?
Maybe.. Those hooks are not broken, the implementation could be improved a bit to have cleaner code. The problem in #19700 comes from not being able to keep a stable reference of the callbacks we pass to these hooks, but that's not a problem of the hook itself. |
|
Little bump here , Since the |
|
Agree, let's close this out |
|
Hey @fedirjh @roryabraham 👋 , I'm coming here because of this TS issue, we must migrate this component to function one first before migrating to TS. I would like to know if the TS Migration team can do it now or, if not, who is going to continue this migration. Thanks! |
|
I think we can/should use the hook going forward |
|
Thanks, I will leave this comment in the issue. |
Details
The ArrowKeyFocusManager is used by:
ReportActionCompose- for emoji and mention suggestionsOptionsSelector- used by many components, e.g.NewChatPageAttachmentPicker/index.native.jsI was not able to get the keyboard keys to work in the iOS and Android simulator (but the same is true for new.expensify.com).
I couldn't get the
OptionsSelectorworking on mWeb versions as well.Fixed Issues
$ #16110
Tests
@:smOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Screen.Recording.2023-05-30.at.13.58.06.mov
Mobile Web - Chrome
Screen.Recording.2023-05-30.at.14.36.48.mov
Mobile Web - Safari
Screen.Recording.2023-05-30.at.14.56.09.mov
Desktop
Screen.Recording.2023-05-30.at.13.58.48.mov
iOS
I was unable to get the keyboard keys to work on iOS
Android
I was unable to get the keyboard keys to work on Android