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: Selecting last possible member to group hides selected members. #34847

Merged
merged 11 commits into from
Feb 1, 2024
Merged

fix: Selecting last possible member to group hides selected members. #34847

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1720478
fix: Selecting last possible member to group hides selected members.
Krishna2323 Jan 20, 2024
1176d22
comment added.
Krishna2323 Jan 20, 2024
89c96b1
minor fix.
Krishna2323 Jan 23, 2024
47e6c4f
fix: ordering of formatSectionsFromSearchTerm defauls parameters.
Krishna2323 Jan 26, 2024
7191323
fix: comments.
Krishna2323 Jan 28, 2024
f924ea0
Merge branch 'main' of https://github.com/Krishna2323/App into krishn…
Krishna2323 Jan 28, 2024
b04e153
Merge branch 'main' of https://github.com/Krishna2323/App into krishn…
Krishna2323 Jan 28, 2024
efa6bfa
fix: code updated due to merge conflicts.
Krishna2323 Jan 28, 2024
40b30f0
comment updated for formatSectionsFromSearchTerm.
Krishna2323 Jan 28, 2024
5b42bdf
update: callback function to hook.
Krishna2323 Jan 31, 2024
ec468ed
Merge branch 'Expensify:main' into krishna2323/issue/34111
Krishna2323 Jan 31, 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
16 changes: 13 additions & 3 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1941,13 +1941,23 @@ function shouldOptionShowTooltip(option) {
* @param {Object} personalDetails
* @param {Boolean} shouldGetOptionDetails
* @param {Number} indexOffset
* @param {Boolean} maxOptionsSelected
* @returns {Object}
*/
function formatSectionsFromSearchTerm(searchTerm, selectedOptions, filteredRecentReports, filteredPersonalDetails, personalDetails = {}, shouldGetOptionDetails = false, indexOffset) {
// We show the selected participants at the top of the list when there is no search term
function formatSectionsFromSearchTerm(
searchTerm,
selectedOptions,
filteredRecentReports,
filteredPersonalDetails,
personalDetails = {},
shouldGetOptionDetails = false,
indexOffset,
maxOptionsSelected,
Krishna2323 marked this conversation as resolved.
Show resolved Hide resolved
) {
// We show the selected participants at the top of the list when there is no search term or maximum number of participants has already been selected
// However, if there is a search term we remove the selected participants from the top of the list unless they are part of the search results
// This clears up space on mobile views, where if you create a group with 4+ people you can't see the selected participants and the search results at the same time
if (searchTerm === '') {
if (searchTerm === '' || maxOptionsSelected) {
return {
section: {
title: undefined,
Expand Down
24 changes: 19 additions & 5 deletions src/pages/NewChatPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,16 @@ function NewChatPage({betas, isGroupChat, personalDetails, reports, translate, i
const sectionsList = [];
let indexOffset = 0;

const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(searchTerm, selectedOptions, filteredRecentReports, filteredPersonalDetails, {}, false, indexOffset);
const formatResults = OptionsListUtils.formatSectionsFromSearchTerm(
searchTerm,
selectedOptions,
filteredRecentReports,
filteredPersonalDetails,
{},
false,
indexOffset,
maxParticipantsReached,
Krishna2323 marked this conversation as resolved.
Show resolved Hide resolved
);
sectionsList.push(formatResults.section);
indexOffset = formatResults.newIndexOffset;

Expand Down Expand Up @@ -230,10 +239,15 @@ function NewChatPage({betas, isGroupChat, personalDetails, reports, translate, i
}, [didScreenTransitionEnd, updateOptions]);

// When search term updates we will fetch any reports
const setSearchTermAndSearchInServer = useCallback((text = '') => {
Report.searchInServer(text);
setSearchTerm(text);
}, []);
const setSearchTermAndSearchInServer = useCallback(
(text = '') => {
if (text && !maxParticipantsReached) {
Report.searchInServer(text);
}
setSearchTerm(text);
},
[maxParticipantsReached],
);

const {inputCallbackRef} = useAutoFocusInput();

Expand Down
16 changes: 10 additions & 6 deletions src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
personalDetails,
true,
indexOffset,
maxParticipantsReached,
);
newSections.push(formatResults.section);
indexOffset = formatResults.newIndexOffset;
Expand Down Expand Up @@ -239,12 +240,15 @@ function MoneyTemporaryForRefactorRequestParticipantsSelector({
);

// When search term updates we will fetch any reports
const setSearchTermAndSearchInServer = useCallback((text = '') => {
if (text.length) {
Report.searchInServer(text);
}
setSearchTerm(text);
}, []);
const setSearchTermAndSearchInServer = useCallback(
(text = '') => {
Krishna2323 marked this conversation as resolved.
Show resolved Hide resolved
if (text.length && !maxParticipantsReached) {
Report.searchInServer(text);
}
setSearchTerm(text);
},
[maxParticipantsReached],
);

// Right now you can't split a request with a workspace and other additional participants
// This is getting properly fixed in https://github.com/Expensify/App/issues/27508, but as a stop-gap to prevent
Expand Down
14 changes: 10 additions & 4 deletions src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ function MoneyRequestParticipantsSelector({
personalDetails,
true,
indexOffset,
maxParticipantsReached,
);
newSections.push(formatResults.section);
indexOffset = formatResults.newIndexOffset;
Expand Down Expand Up @@ -259,10 +260,15 @@ function MoneyRequestParticipantsSelector({
);

// When search term updates we will fetch any reports
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also update this comment to explain the new changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also since this is pretty much copy / pasted in 3 files, should we make it a reusable lib?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we should do both.

cc: @Krishna2323

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c3024, comments have been updated, can you help me understanding what do you mean by making it reusable lib, the setSearchTermAndSearchInServer uses useCallback & setState.

Copy link
Contributor

Choose a reason for hiding this comment

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

We extract this

    const setSearchTermAndSearchInServer = useCallback(
        (text = '') => {
            if (text && !maxParticipantsReached) {
                Report.searchInServer(text);
            }
            setSearchTerm(text);
        },
        [maxParticipantsReached],
    );

to a function in OptionListUtils.ts (or a more appropriate file) and use it in the three pages. @Krishna2323

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c3024, I believe it will over complicate things, as we need to use useCallback because we pass it to OptionsSelector.

Otherwise the option seems to create a util function that returns a function which will be used inside useCallback but as I said it will complicate things. Current implementation looks simpler to me and its just few lines of code, so I don't think we should extract few lines and still use useCallback which is a must.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about a custom hook like this?

import { useCallback } from 'react';
import * as Report from '@userActions/Report';

const useSearchTermAndSearch = (setSearchTerm, maxParticipantsReached) => {
    const setSearchTermAndSearchInServer = useCallback(
        (text = '') => {
            if (text && !maxParticipantsReached) {
                Report.searchInServer(text);
            }
            setSearchTerm(text);
        },
        [maxParticipantsReached, setSearchTerm]
    );

    return setSearchTermAndSearchInServer;
};

export default useSearchTermAndSearch;

and use it in the pages like this

const setSearchTermAndSearchInServer = useSearchTermAndSearch(setSearchTerm, maxParticipantsReached);

WDYT? @Beamanator

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that plan @c3024 👍 looks like we have a decent amount of custom hooks in the src/hooks directory, so this would be a nice one to add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c3024, can you pls review once, created the hook and tested.

const setSearchTermAndSearchInServer = useCallback((text = '') => {
Report.searchInServer(text);
setSearchTerm(text);
}, []);
const setSearchTermAndSearchInServer = useCallback(
(text = '') => {
if (text && !maxParticipantsReached) {
Report.searchInServer(text);
}
setSearchTerm(text);
},
[maxParticipantsReached],
);

// Right now you can't split a request with a workspace and other additional participants
// This is getting properly fixed in https://github.com/Expensify/App/issues/27508, but as a stop-gap to prevent
Expand Down
Loading