Fix search type popover has 2 items highlighted #53330
Conversation
|
I didn't update this -1 focused index for the sub menu back button because I think if there is a sub-menu, then the parent menu won't have a selected option. App/src/components/PopoverMenu.tsx Lines 236 to 239 in 1a6f48e For this one, when we select the item, the popover will be closed or entered in the submenu. I didn't touch this one too because we already reset the focused index when the modal hides. App/src/components/PopoverMenu.tsx Lines 296 to 297 in 1a6f48e |
src/components/PopoverMenu.tsx
Outdated
| useEffect(() => { | ||
| if (isVisible) { | ||
| return; | ||
| } | ||
| setFocusedIndex(currentMenuItemsFocusedIndex); | ||
| }, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]); |
There was a problem hiding this comment.
currentMenuItemsFocusedIndex is changed due to the change in currentMenuItems which is due to the call setCurrentMenuItems(menuItems) that is done in the above useLayoutEffect. This is a known change and not a side effect and it should be done as soon as possible, i.e. call setFocusedIndex right after setCurrentMenuItems.
Note: Keep the isVisible condition but add a comment that we don't want the focus index to change if we dynamically change the items e.g.
If we have:
- Item 1 (Selected)
- Item 2 (Focused)
- Item 3
then the selected item is changed (externally), the result should be as follow:
- Item 1
- Item 2 (Focused)
- Item 3 (Selected)
There was a problem hiding this comment.
Added comment with the help from ChatGPT.
There was a problem hiding this comment.
Can you move the setFocusedIndex call as well
There was a problem hiding this comment.
Sorry, move to where? And why? I don't quite understand your first comment actually 😅
There was a problem hiding this comment.
Move it to the above useLayoutEffect
There was a problem hiding this comment.
Oh, I get what you're trying to say now. But if we do that, then we shouldn't add isVisible to the useLayoutEffect deps right?
useLayoutEffect(() => {
if (menuItems.length === 0) {
return;
}
setEnteredSubMenuIndexes(CONST.EMPTY_ARRAY);
setCurrentMenuItems(menuItems);
if (isVisible) {
return;
}
setFocusedIndex(currentMenuItemsFocusedIndex);
// eslint disable
}, [menuItems, currentMenuItemsFocusedIndex, setFocusedIndex]);
Also, I don't think this would work. Why? When we call setCurrentMenuItems(menuItems);, currentMenuItemsFocusedIndex is not updated yet, so calling setFocusedIndex(currentMenuItemsFocusedIndex); right after it won't work. It'll be updated correctly after the 2nd trigger of useLayoutEffect.
There was a problem hiding this comment.
we shouldn't add isVisible to the useLayoutEffect deps right
Right. I don't think this is needed or wanted.
Also, I don't think this would work
Remove the currentMenuItemsFocusedIndex const and just use menuItems?.findIndex((option) => option.isSelected).
This is one of the cases where a useEffect is not needed.
Bad:
const [value, setValue] = useState(7);
const [derivedValue, setDerivedValue] = useState(13);
const changeValue = () => {
setValue(8);
};
useEffect(() => {
setDerivedValue(21);
}, [value])Not as bad:
const [value, setValue] = useState(7);
const [derivedValue, setDerivedValue] = useState(13);
const changeValue = () => {
setValue(8);
setDerivedValue(21);
};Good:
const [value, setValue] = useState(7);
const derivedValue = fibo(value);There was a problem hiding this comment.
In our case we are trying to move from Bad to Not as bad. value=menuItems, derivedValue=focusedIndex
There was a problem hiding this comment.
Got it now, updated
src/components/PopoverMenu.tsx
Outdated
| 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. | ||
| setFocusedIndex(getSelectedItemIndex(menuItems)); |
There was a problem hiding this comment.
| 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. | |
| setFocusedIndex(getSelectedItemIndex(menuItems)); | |
| // 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)); |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.0.72-0 🚀
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.0.72-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.72-1 🚀
|
Explanation of Change
Fixed Issues
$ #50224
PROPOSAL: #50224 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have a saved search
For web/desktop: resize it to a small screen
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
android.mp4
Android: mWeb Chrome
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mwe.b.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4