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

Jump to bottom

fix: Recalculate the sections only on actual change #37341

Merged
merged 4 commits into from
Feb 28, 2024
Merged

fix: Recalculate the sections only on actual change #37341

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7ca401
Recalculate the sections only on actual change
paultsimura Feb 27, 2024
8e816ff
Set sections to initial value on render
paultsimura Feb 27, 2024
b39957f
Initialize sections with a value
paultsimura Feb 27, 2024
181c21f
Merge branch 'main' into fix/37238-selection-index
paultsimura Feb 27, 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
14 changes: 12 additions & 2 deletions src/components/OptionsSelector/BaseOptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {useIsFocused} from '@react-navigation/native';
import lodashGet from 'lodash/get';
import lodashIsEqual from 'lodash/isEqual';
import PropTypes from 'prop-types';
import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react';
import {ScrollView, View} from 'react-native';
Expand All @@ -14,6 +15,7 @@ import ShowMoreButton from '@components/ShowMoreButton';
import TextInput from '@components/TextInput';
import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import useLocalize from '@hooks/useLocalize';
import usePrevious from '@hooks/usePrevious';
import useThemeStyles from '@hooks/useThemeStyles';
import getPlatform from '@libs/getPlatform';
import KeyboardShortcut from '@libs/KeyboardShortcut';
Expand Down Expand Up @@ -100,6 +102,7 @@ function BaseOptionsSelector(props) {
const prevPaginationPage = useRef(paginationPage);
const prevSelectedOptions = useRef(props.selectedOptions);
const prevValue = useRef(value);
const previousSections = usePrevious(props.sections);

useImperativeHandle(props.forwardedRef, () => textInputRef.current);

Expand Down Expand Up @@ -143,7 +146,7 @@ function BaseOptionsSelector(props) {
/**
* Maps sections to render only allowed count of them per section.
*
* @returns {Objects[]}
* @returns {Object[]}
*/
const sliceSections = useCallback(
() =>
Expand Down Expand Up @@ -343,6 +346,9 @@ function BaseOptionsSelector(props) {
[allOptions, sections],
);

// eslint-disable-next-line react-hooks/exhaustive-deps
useEffect(() => setSections(sliceSections()), []);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure if we shouldn't just use this instead:

    const [sections, setSections] = useState(sliceSections());

It will uglify the ordering of the useState/useCallback in the file, but does essentially the same.
Would appreciate your thought @akinwale.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer useState here, since it makes the sections available for the first render, instead of right after the first render which is what happens with useEffect.


useEffect(() => {
subscribeToEnterShortcut();
subscribeToCtrlEnterShortcut();
Expand Down Expand Up @@ -415,6 +421,10 @@ function BaseOptionsSelector(props) {
}, [isFocused, props.autoFocus]);

useEffect(() => {
if (lodashIsEqual(props.sections, previousSections)) {
return;
}

const newSections = sliceSections();
const newOptions = flattenSections();

Expand All @@ -434,7 +444,7 @@ function BaseOptionsSelector(props) {
setFocusedIndex(prevFocusedOptionIndex || (_.isNumber(props.focusedIndex) ? props.focusedIndex : newFocusedIndex));
// we want to run this effect only when the sections change
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.sections]);
}, [props.sections, previousSections]);

useEffect(() => {
// If we just toggled an option on a multi-selection page or cleared the search input, scroll to top
Expand Down
Loading