From dc8a2db51b679c71134de901b34e257e2cfe266f Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 30 Nov 2024 15:14:55 +0800 Subject: [PATCH 1/9] update the focused index whilet the popover is hidden if the selected item is changed --- src/components/PopoverMenu.tsx | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 9b5c0b1b6f56b..4c4f55e7d8252 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -1,7 +1,7 @@ /* eslint-disable react/jsx-props-no-spreading */ import lodashIsEqual from 'lodash/isEqual'; import type {ReactNode, RefObject} from 'react'; -import React, {useLayoutEffect, useState} from 'react'; +import React, {useEffect, useLayoutEffect, useState} from 'react'; import {StyleSheet, View} from 'react-native'; import type {StyleProp, TextStyle, ViewStyle} from 'react-native'; import type {ModalProps} from 'react-native-modal'; @@ -314,6 +314,13 @@ function PopoverMenu({ setCurrentMenuItems(menuItems); }, [menuItems]); + useEffect(() => { + if (isVisible) { + return; + } + setFocusedIndex(currentMenuItemsFocusedIndex); + }, [isVisible, currentMenuItemsFocusedIndex]); + return ( Date: Sat, 30 Nov 2024 15:15:26 +0800 Subject: [PATCH 2/9] reset the focused index to the selected item if any when the popover hides --- src/components/PopoverMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 4c4f55e7d8252..0876cbcf48510 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -300,7 +300,7 @@ function PopoverMenu({ ); const onModalHide = () => { - setFocusedIndex(-1); + setFocusedIndex(currentMenuItemsFocusedIndex); }; // When the menu items are changed, we want to reset the sub-menu to make sure From 5bad35f0fbed416f8d23f0cabd04a02d93fce1c5 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sat, 30 Nov 2024 15:25:17 +0800 Subject: [PATCH 3/9] lint --- src/components/PopoverMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 0876cbcf48510..f2871fb72fb87 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -319,7 +319,7 @@ function PopoverMenu({ return; } setFocusedIndex(currentMenuItemsFocusedIndex); - }, [isVisible, currentMenuItemsFocusedIndex]); + }, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]); return ( Date: Sun, 1 Dec 2024 16:26:01 +0800 Subject: [PATCH 4/9] add comment --- src/components/PopoverMenu.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index f2871fb72fb87..24f49a5be8e92 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -318,6 +318,10 @@ function PopoverMenu({ if (isVisible) { return; } + + // Update the focused item to match the selected item, but only when the popover is not visible. + // This ensures that if the popover is visible, highlight from the keyboard navigation is not overridden + // by external updates to the selected item, preventing user experience disruption. setFocusedIndex(currentMenuItemsFocusedIndex); }, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]); From 95803faa60a117fc0187b45fbac8138221dc9fd1 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Sun, 1 Dec 2024 22:10:38 +0800 Subject: [PATCH 5/9] update comment --- src/components/PopoverMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 24f49a5be8e92..c628a00a89a19 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -321,7 +321,7 @@ function PopoverMenu({ // Update the focused item to match the selected item, but only when the popover is not visible. // This ensures that if the popover is visible, highlight from the keyboard navigation is not overridden - // by external updates to the selected item, preventing user experience disruption. + // by external updates. setFocusedIndex(currentMenuItemsFocusedIndex); }, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]); From 539ca23e46e658ac5667b0ab6db5814cecda970c Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 4 Dec 2024 21:51:41 +0800 Subject: [PATCH 6/9] simplify code --- src/components/PopoverMenu.tsx | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index c628a00a89a19..0f32a5164b0a7 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -137,6 +137,10 @@ const renderWithConditionalWrapper = (shouldUseScrollView: boolean, contentConta return <>{children}; }; +function getSelectedItemIndex(menuItems: PopoverMenuItem[]) { + return menuItems.findIndex((option) => option.isSelected); +} + function PopoverMenu({ menuItems, onItemSelected, @@ -174,7 +178,7 @@ function PopoverMenu({ // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth const {isSmallScreenWidth} = useResponsiveLayout(); const [currentMenuItems, setCurrentMenuItems] = useState(menuItems); - const currentMenuItemsFocusedIndex = currentMenuItems?.findIndex((option) => option.isSelected); + const currentMenuItemsFocusedIndex = getSelectedItemIndex(currentMenuItems); const [enteredSubMenuIndexes, setEnteredSubMenuIndexes] = useState(CONST.EMPTY_ARRAY); const {windowHeight} = useWindowDimensions(); @@ -312,9 +316,7 @@ function PopoverMenu({ } setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); setCurrentMenuItems(menuItems); - }, [menuItems]); - useEffect(() => { if (isVisible) { return; } @@ -322,8 +324,10 @@ function PopoverMenu({ // Update the focused item to match the selected item, but only when the popover is not visible. // This ensures that if the popover is visible, highlight from the keyboard navigation is not overridden // by external updates. - setFocusedIndex(currentMenuItemsFocusedIndex); - }, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]); + setFocusedIndex(getSelectedItemIndex(menuItems)); + + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [menuItems, setFocusedIndex]); return ( Date: Wed, 4 Dec 2024 23:28:06 +0800 Subject: [PATCH 7/9] lint --- src/components/PopoverMenu.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 0f32a5164b0a7..556f05096c3b0 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -1,7 +1,7 @@ /* eslint-disable react/jsx-props-no-spreading */ import lodashIsEqual from 'lodash/isEqual'; import type {ReactNode, RefObject} from 'react'; -import React, {useEffect, useLayoutEffect, useState} from 'react'; +import React, {useLayoutEffect, useState} from 'react'; import {StyleSheet, View} from 'react-native'; import type {StyleProp, TextStyle, ViewStyle} from 'react-native'; import type {ModalProps} from 'react-native-modal'; @@ -326,7 +326,7 @@ function PopoverMenu({ // by external updates. setFocusedIndex(getSelectedItemIndex(menuItems)); - // eslint-disable-next-line react-hooks/exhaustive-deps + // eslint-disable-next-line react-compiler/react-compiler react-hooks/exhaustive-deps }, [menuItems, setFocusedIndex]); return ( From e5ba8d88755abb43674b12ebe6ecf453db4178ec Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 4 Dec 2024 23:31:25 +0800 Subject: [PATCH 8/9] lint --- src/components/PopoverMenu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 556f05096c3b0..6025314b36b27 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -326,7 +326,7 @@ function PopoverMenu({ // by external updates. setFocusedIndex(getSelectedItemIndex(menuItems)); - // eslint-disable-next-line react-compiler/react-compiler react-hooks/exhaustive-deps + // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps }, [menuItems, setFocusedIndex]); return ( From 11ba3771273ef333e9d502193da761df8c71198f Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Wed, 4 Dec 2024 23:36:48 +0800 Subject: [PATCH 9/9] move comment --- src/components/PopoverMenu.tsx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/components/PopoverMenu.tsx b/src/components/PopoverMenu.tsx index 6025314b36b27..f5b430499300a 100644 --- a/src/components/PopoverMenu.tsx +++ b/src/components/PopoverMenu.tsx @@ -317,13 +317,12 @@ function PopoverMenu({ setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY); setCurrentMenuItems(menuItems); - if (isVisible) { - return; - } - // Update the focused item to match the selected item, but only when the popover is not visible. // This ensures that if the popover is visible, highlight from the keyboard navigation is not overridden // by external updates. + if (isVisible) { + return; + } setFocusedIndex(getSelectedItemIndex(menuItems)); // eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps