Skip to content
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.

Sign up for GitHub

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

Fix search type popover has 2 items highlighted #53330

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Fix search type popover has 2 items highlighted #53330

Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dc8a2db
update the focused index whilet the popover is hidden if the selected…
bernhardoj Nov 30, 2024
bf580a2
reset the focused index to the selected item if any when the popover …
bernhardoj Nov 30, 2024
5bad35f
lint
bernhardoj Nov 30, 2024
c564f3c
add comment
bernhardoj Dec 1, 2024
95803fa
update comment
bernhardoj Dec 1, 2024
dddbed1
Merge branch 'main' into fix/50224-search-type-popover-has-2-items-hi…
bernhardoj Dec 4, 2024
539ca23
simplify code
bernhardoj Dec 4, 2024
30ff2ca
lint
bernhardoj Dec 4, 2024
e5ba8d8
lint
bernhardoj Dec 4, 2024
11ba377
move comment
bernhardoj Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/components/PopoverMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -314,6 +314,13 @@ function PopoverMenu({
setCurrentMenuItems(menuItems);
}, [menuItems]);

useEffect(() => {
if (isVisible) {
return;
}
setFocusedIndex(currentMenuItemsFocusedIndex);
}, [isVisible, currentMenuItemsFocusedIndex, setFocusedIndex]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment with the help from ChatGPT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the setFocusedIndex call as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, move to where? And why? I don't quite understand your first comment actually 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move it to the above useLayoutEffect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@s77rt s77rt Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our case we are trying to move from Bad to Not as bad. value=menuItems, derivedValue=focusedIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it now, updated


return (
<PopoverWithMeasuredContent
anchorPosition={anchorPosition}
Expand Down
Loading