-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync keyboard navigation in SelectionList and PopoverMenu #39201
Changes from 15 commits
4b11894
e7c7817
ef22ff2
178b927
f91e6d0
519c0a2
f470b96
cfe13d5
315f815
ead8292
a8fd58c
0d1f5b0
a3c3116
85f75a7
52b93c9
3f4072c
9bf097d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import React, {useRef} from 'react'; | ||
import type {View} from 'react-native'; | ||
import useSyncFocus from '@hooks/useSyncFocus'; | ||
import type {MenuItemProps} from './MenuItem'; | ||
import MenuItem from './MenuItem'; | ||
|
||
function FocusableMenuItem(props: MenuItemProps) { | ||
const ref = useRef<HTMLDivElement | View>(null); | ||
|
||
// Sync focus on an item | ||
useSyncFocus(ref, Boolean(props.focused)); | ||
|
||
return ( | ||
<MenuItem | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} | ||
ref={ref} | ||
/> | ||
); | ||
} | ||
|
||
FocusableMenuItem.displayName = 'FocusableMenuItem'; | ||
|
||
export default FocusableMenuItem; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
import React from 'react'; | ||
import React, {useRef} from 'react'; | ||
import {View} from 'react-native'; | ||
import Icon from '@components/Icon'; | ||
import * as Expensicons from '@components/Icon/Expensicons'; | ||
import OfflineWithFeedback from '@components/OfflineWithFeedback'; | ||
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback'; | ||
import useHover from '@hooks/useHover'; | ||
import useSyncFocus from '@hooks/useSyncFocus'; | ||
import useTheme from '@hooks/useTheme'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import CONST from '@src/CONST'; | ||
|
@@ -26,11 +27,18 @@ function BaseListItem<TItem extends ListItem>({ | |
pendingAction, | ||
FooterComponent, | ||
children, | ||
isFocused, | ||
onFocus = () => {}, | ||
}: BaseListItemProps<TItem>) { | ||
const theme = useTheme(); | ||
const styles = useThemeStyles(); | ||
const {hovered, bind} = useHover(); | ||
|
||
const pressableRef = useRef<View | HTMLDivElement>(null); | ||
|
||
// Sync focus on an item | ||
useSyncFocus(pressableRef, Boolean(isFocused)); | ||
|
||
const rightHandSideComponentRender = () => { | ||
if (canSelectMultiple || !rightHandSideComponent) { | ||
return null; | ||
|
@@ -54,6 +62,7 @@ function BaseListItem<TItem extends ListItem>({ | |
<PressableWithFeedback | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...bind} | ||
ref={pressableRef} | ||
onPress={() => onSelectRow(item)} | ||
disabled={isDisabled} | ||
accessibilityLabel={item.text ?? ''} | ||
|
@@ -64,6 +73,7 @@ function BaseListItem<TItem extends ListItem>({ | |
onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (e) => e.preventDefault() : undefined} | ||
nativeID={keyForList ?? ''} | ||
style={pressableStyle} | ||
onFocus={onFocus} | ||
> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coming from this issue #41922, we have added the onFocus to BaseListItem in this PR. We already had the onMouseDown conditional prop passed, so the bug might have been there with the conditional onMouseDown prop but became visible after the onFocus change. When we long press the currency in CurrencySelection, it triggers the onFocus function, and we see a scroll to the long-pressed index. We have fixed this by removing the conditional onMouseDown in BaseListItem. |
||
<View style={wrapperStyle}> | ||
{typeof children === 'function' ? children(hovered) : children} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great find 👍