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.

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

Added allOptions to scrollToIndex callback dependencies #29848

Merged
Changes from 5 commits
Commits
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
40 changes: 20 additions & 20 deletions src/components/SelectionList/BaseSelectionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,31 +152,31 @@ function BaseSelectionList({
* @param {Number} index - the index of the item to scroll to
* @param {Boolean} animated - whether to animate the scroll
*/
const scrollToIndex = useCallback((index, animated = true) => {
const item = flattenedSections.allOptions[index];
const scrollToIndex = useCallback(
(index, animated = true) => {
const item = flattenedSections.allOptions[index];

if (!listRef.current || !item) {
return;
}
if (!listRef.current || !item) {
return;
}

const itemIndex = item.index;
const sectionIndex = item.sectionIndex;
const itemIndex = item.index;
const sectionIndex = item.sectionIndex;

// Note: react-native's SectionList automatically strips out any empty sections.
// So we need to reduce the sectionIndex to remove any empty sections in front of the one we're trying to scroll to.
// Otherwise, it will cause an index-out-of-bounds error and crash the app.
let adjustedSectionIndex = sectionIndex;
for (let i = 0; i < sectionIndex; i++) {
if (_.isEmpty(lodashGet(sections, `[${i}].data`))) {
adjustedSectionIndex--;
// Note: react-native's SectionList automatically strips out any empty sections.
// So we need to reduce the sectionIndex to remove any empty sections in front of the one we're trying to scroll to.
// Otherwise, it will cause an index-out-of-bounds error and crash the app.
let adjustedSectionIndex = sectionIndex;
for (let i = 0; i < sectionIndex; i++) {
if (_.isEmpty(lodashGet(sections, `[${i}].data`))) {
adjustedSectionIndex--;
}
}
}

listRef.current.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated, viewOffset: variables.contentHeaderHeight});

// If we don't disable dependencies here, we would need to make sure that the `sections` prop is stable in every usage of this component.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
listRef.current.scrollToLocation({sectionIndex: adjustedSectionIndex, itemIndex, animated, viewOffset: variables.contentHeaderHeight});
},
[flattenedSections.allOptions, sections],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rushatgabhane Getting below error if we pass only flattenedSections.allOptions to dependencies, so to solve that warning, I've added the sections to the dependencies; not sure if this is a good idea as this line (we would need to make sure that the sections prop is stable in every usage of this component)
suggests we should check if sections are stable in every usage, right now SelectionList is being used at 14 pages.
Screenshot 2023-10-18 at 4 43 47 PM

Copy link
Member

@rushatgabhane rushatgabhane Oct 20, 2023

Choose a reason for hiding this comment

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

@jayeshmangwani can't we exclude sections from the deps? and slap eslint supress on it

// eslint-disable-next-line react-hooks/exhaustive-deps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rushatgabhane Thanks for the suggestion. It would also work. I have added disable exhaustive-deps and pushed code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rushatgabhane Applied suggested changes and merged the main branch; please help to review

);

/**
* Logic to run when a row is selected, either with click/press or keyboard hotkeys.
Expand Down
Loading