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/29901: Selected category should be in the list #32902

Merged
merged 10 commits into from
Jan 3, 2024
Merged

Fix/29901: Selected category should be in the list #32902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ea7e89a
add selected category first
DylanDylann Dec 12, 2023
a9bf6d6
add selected category once
DylanDylann Dec 12, 2023
61a83f4
fix lint
DylanDylann Dec 12, 2023
3140d05
merge main
DylanDylann Dec 14, 2023
910e9bf
merge main
DylanDylann Dec 18, 2023
477c3a3
fix lint
DylanDylann Dec 18, 2023
7a914e8
merge main
DylanDylann Dec 25, 2023
7039ed9
fix margin bug
DylanDylann Dec 25, 2023
3abcdfb
fix margin bug
DylanDylann Dec 25, 2023
19891e1
fix margin bug
DylanDylann Dec 25, 2023
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
25 changes: 13 additions & 12 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,31 +847,34 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
return categorySections;
}

if (numberOfCategories < CONST.CATEGORY_LIST_THRESHOLD) {
if (!_.isEmpty(selectedOptions)) {
categorySections.push({
// "All" section when items amount less than the threshold
// "Selected" section
title: '',
shouldShow: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

ok so true > false change is fine but curious why this empty section header is needed when title is empty.

App/src/components/OptionsList/BaseOptionsList.js

Lines 236 to 238 in 595bf40

if (!title && shouldShow && !hideSectionHeaders && sectionHeaderStyle) {
return <View style={sectionHeaderStyle} />;
}

Have you found any case this is necessary?

Copy link
Contributor Author

@DylanDylann DylanDylann Dec 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@aimane-chnaif I see that we also do the same thing in here

App/src/libs/OptionsListUtils.js

Lines 824 to 828 in 595bf40

if (numberOfCategories === 0 && selectedOptions.length > 0) {
categorySections.push({
// "Selected" section
title: '',
shouldShow: false,

so that I update shouldshow : true > false to make it consistency

indexOffset,
data: getCategoryOptionTree(enabledCategories),
data: getCategoryOptionTree(selectedOptions, true),
});

return categorySections;
indexOffset += selectedOptions.length;
}

if (!_.isEmpty(selectedOptions)) {
const selectedOptionNames = _.map(selectedOptions, (selectedOption) => selectedOption.name);
const filteredCategories = _.filter(enabledCategories, (category) => !_.includes(selectedOptionNames, category.name));
const numberOfVisibleCategories = selectedOptions.length + filteredCategories.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have checked if the selectedOptions were enabled or not. This later caused #37766


if (numberOfVisibleCategories < CONST.CATEGORY_LIST_THRESHOLD) {
categorySections.push({
// "Selected" section
// "All" section when items amount less than the threshold
title: '',
shouldShow: true,
shouldShow: false,
indexOffset,
data: getCategoryOptionTree(selectedOptions, true),
data: getCategoryOptionTree(filteredCategories),
});

indexOffset += selectedOptions.length;
return categorySections;
}

const selectedOptionNames = _.map(selectedOptions, (selectedOption) => selectedOption.name);
const filteredRecentlyUsedCategories = _.chain(recentlyUsedCategories)
.filter((categoryName) => !_.includes(selectedOptionNames, categoryName) && lodashGet(categories, [categoryName, 'enabled'], false))
.map((categoryName) => ({
Expand All @@ -894,8 +897,6 @@ function getCategoryListSections(categories, recentlyUsedCategories, selectedOpt
indexOffset += filteredRecentlyUsedCategories.length;
}

const filteredCategories = _.filter(enabledCategories, (category) => !_.includes(selectedOptionNames, category.name));

categorySections.push({
// "All" section when items amount more than the threshold
title: Localize.translateLocal('common.all'),
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/OptionsListUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,7 @@ describe('OptionsListUtils', () => {
const largeResultList = [
{
title: '',
shouldShow: true,
shouldShow: false,
indexOffset: 0,
data: [
{
Expand Down
Loading