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

Migrate MoneyRequestParticipantsSplitSelector.js to function component #20271

Merged
merged 16 commits into from
Jul 4, 2023
Merged

Migrate MoneyRequestParticipantsSplitSelector.js to function component #20271

Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f8661e7
Migrate MoneyRequestParticipantsSplitSelector.js to function component
multijump Jun 6, 2023
8ecd4fb
Fixed lint issues
multijump Jun 6, 2023
0b66de4
Revert podfile.lock
multijump Jun 6, 2023
e10cd88
Fix
multijump Jun 6, 2023
35b3f79
added translate per reviewer request
multijump Jun 9, 2023
4c375f9
fixed lint issues
multijump Jun 12, 2023
041d782
fixed conflicts from main branch
multijump Jun 15, 2023
c068bfb
Merge branch 'main' of github.com-avastorm:multijump/Expensify-App in…
multijump Jun 15, 2023
4dd5f0a
restore function orders per reviewer's request
multijump Jun 20, 2023
ffd6fad
fixed prettier issue
multijump Jun 20, 2023
49252c1
Merge branch 'main' of github.com-avastorm:multijump/Expensify-App in…
multijump Jun 20, 2023
82be71e
Updated per reviewer's requests
multijump Jun 20, 2023
a1e5ba1
fixed lint issue
multijump Jun 20, 2023
05a9937
Merge branch 'main' into 16280_refactor_MoneyRequestParticipantsSpli…
multijump Jun 22, 2023
0afb3e6
Merge branch 'main' of github.com-avastorm:multijump/Expensify-App in…
multijump Jun 25, 2023
b70b37e
Merge branch 'main' into 16280_refactor_MoneyRequestParticipantsSpli…
multijump Jun 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
236 changes: 105 additions & 131 deletions src/pages/iou/steps/MoneyRequstParticipantsPage/MoneyRequestParticipantsSplitSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, {Component} from 'react';
import React, {useEffect, useState} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
Expand Down Expand Up @@ -60,183 +60,157 @@ const defaultProps = {
safeAreaPaddingBottomStyle: {},
};

class MoneyRequestParticipantsSplitSelector extends Component {
constructor(props) {
super(props);

this.toggleOption = this.toggleOption.bind(this);
this.finalizeParticipants = this.finalizeParticipants.bind(this);
this.updateOptionsWithSearchTerm = this.updateOptionsWithSearchTerm.bind(this);

const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
props.reports,
props.personalDetails,
props.betas,
'',
props.participants,
CONST.EXPENSIFY_EMAILS,
);

this.state = {
searchTerm: '',
recentReports,
personalDetails,
userToInvite,
};
}

componentDidUpdate(prevProps) {
if (_.isEqual(prevProps.reports, this.props.reports) && _.isEqual(prevProps.personalDetails, this.props.personalDetails)) {
return;
}
this.updateOptionsWithSearchTerm(this.state.searchTerm);
}
function MoneyRequestParticipantsSplitSelector(props) {
const [searchTerm, setSearchTerm] = useState('');
const [newChatOptions, setNewChatOptions] = useState({
recentReports: [],
personalDetails: [],
userToInvite: null,
});

/**
* Returns the sections needed for the OptionsSelector
*
* @param {Boolean} maxParticipantsReached
* @returns {Array}
*/
getSections(maxParticipantsReached) {
const sections = [];
const getSections = (maxParticipantsReached) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to wrap this inside useCallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getSections is not callback function so I think it is not good to use the useCallback.
How about to use the useMemo?

const newSections = [];
let indexOffset = 0;

sections.push({
newSections.push({
title: undefined,
data: this.props.participants,
data: props.participants,
shouldShow: true,
indexOffset,
});
indexOffset += this.props.participants.length;
indexOffset += props.participants.length;

if (maxParticipantsReached) {
return sections;
return newSections;
}

sections.push({
title: this.props.translate('common.recents'),
data: this.state.recentReports,
shouldShow: !_.isEmpty(this.state.recentReports),
const {recentReports, personalDetails, userToInvite} = newChatOptions;

newSections.push({
title: props.translate('common.recents'),
data: recentReports,
shouldShow: !_.isEmpty(recentReports),
indexOffset,
});
indexOffset += this.state.recentReports.length;
indexOffset += recentReports.length;

sections.push({
title: this.props.translate('common.contacts'),
data: this.state.personalDetails,
shouldShow: !_.isEmpty(this.state.personalDetails),
newSections.push({
title: props.translate('common.contacts'),
data: personalDetails,
shouldShow: !_.isEmpty(personalDetails),
indexOffset,
});
indexOffset += this.state.personalDetails.length;
indexOffset += personalDetails.length;

if (this.state.userToInvite && !OptionsListUtils.isCurrentUser(this.state.userToInvite)) {
sections.push({
if (userToInvite && !OptionsListUtils.isCurrentUser(userToInvite)) {
newSections.push({
undefined,
data: [this.state.userToInvite],
data: [userToInvite],
shouldShow: true,
indexOffset,
});
}

return sections;
}

updateOptionsWithSearchTerm(searchTerm = '') {
const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
this.props.reports,
this.props.personalDetails,
this.props.betas,
searchTerm,
this.props.participants,
CONST.EXPENSIFY_EMAILS,
);
this.setState({
searchTerm,
userToInvite,
recentReports,
personalDetails,
});
}

/**
* Once a single or more users are selected, navigates to next step
*/
finalizeParticipants() {
this.props.onStepComplete();
}
return newSections;
};

/**
* Removes a selected option from list if already selected. If not already selected add this option to the list.
* @param {Object} option
*/
toggleOption(option) {
const isOptionInList = _.some(this.props.participants, (selectedOption) => selectedOption.accountID === option.accountID);
const toggleOption = (option) => {
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 we need to wrap this inside useCallback hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped inside useCallback.

const isOptionInList = _.some(props.participants, (selectedOption) => selectedOption.accountID === option.accountID);

let newSelectedOptions;

if (isOptionInList) {
newSelectedOptions = _.reject(this.props.participants, (selectedOption) => selectedOption.accountID === option.accountID);
newSelectedOptions = _.reject(props.participants, (selectedOption) => selectedOption.accountID === option.accountID);
} else {
newSelectedOptions = [...this.props.participants, option];
newSelectedOptions = [...props.participants, option];
}

this.props.onAddParticipants(newSelectedOptions);

this.setState((prevState) => {
const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
this.props.reports,
this.props.personalDetails,
this.props.betas,
isOptionInList ? prevState.searchTerm : '',
newSelectedOptions,
CONST.EXPENSIFY_EMAILS,
);
return {
recentReports,
personalDetails,
userToInvite,
searchTerm: isOptionInList ? prevState.searchTerm : '',
};
});
}

render() {
const maxParticipantsReached = this.props.participants.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;
const sections = this.getSections(maxParticipantsReached);
const headerMessage = OptionsListUtils.getHeaderMessage(
this.state.personalDetails.length + this.state.recentReports.length !== 0,
Boolean(this.state.userToInvite),
this.state.searchTerm,
maxParticipantsReached,
const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
props.reports,
props.personalDetails,
props.betas,
isOptionInList ? searchTerm : '',
newSelectedOptions,
CONST.EXPENSIFY_EMAILS,
);
const isOptionsDataReady = ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(this.props.personalDetails);

return (
<View style={[styles.flex1, styles.w100, this.props.participants.length > 0 ? this.props.safeAreaPaddingBottomStyle : {}]}>
<OptionsSelector
canSelectMultipleOptions
sections={sections}
selectedOptions={this.props.participants}
value={this.state.searchTerm}
onSelectRow={this.toggleOption}
onChangeText={this.updateOptionsWithSearchTerm}
headerMessage={headerMessage}
boldStyle
shouldShowConfirmButton
confirmButtonText={this.props.translate('common.next')}
onConfirmSelection={this.finalizeParticipants}
textInputLabel={this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
safeAreaPaddingBottomStyle={this.props.safeAreaPaddingBottomStyle}
shouldShowOptions={isOptionsDataReady}
/>
</View>

setNewChatOptions({
recentReports,
personalDetails,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to set searchTerm here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we didn't.
Because we have the separated setSearchTerm for set searchTerm state and we don't need to set the searchTerm state here.(because searchTerm wasn't changed in this function)
searchTerm only changes when user type search keyword.
So we don't need to set searchTerm state here.

Copy link
Contributor

Choose a reason for hiding this comment

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

But in the original code, we're resetting the search term if option is in the list. Why were we doing that then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right.
It was my mistake.
Added it.
Thanks @allroundexperts

userToInvite,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use this inside the callback like it was being done previously.

Copy link
Contributor Author

@multijump multijump Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@allroundexperts
This setState doesn't depend on previous state so previousState will be the unused variable.
Do we need to implement to use the callback?

props.onAddParticipants(newSelectedOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you changing the order here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @allroundexperts
There was not special reason.
I thought simply props change (for participants) doesn't affect to toggleOption function running so it doesn't matter where it is.
But I thought changing order was more readable and understandable.
If you require to change the order, I will change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very difficult to compare the before and after code with the order changed. Can you please change the order as it was previously? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @allroundexperts
Restored order per your request.

};

/**
* Once a single or more users are selected, navigates to next step
*/
const finalizeParticipants = () => {
props.onStepComplete();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, why is the order being changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @allroundexperts
There is no reason.
I paid more attention to convert toggleOption and getSections function because these functions are main functions in this component.
So I converted them first and then finalizeParticipants.
That's why changed the order.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, why don't we use withCallback here? I assume it's not necessary, but I'm still trying to understand hooks.

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 we don't need to even wrap this inside the function since the function does nothing other than calling the function supplied in the prop. @multijump can you please remove this and use the prop directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @allroundexperts
Updated to use the prop directly.


const maxParticipantsReached = props.participants.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;
const headerMessage = OptionsListUtils.getHeaderMessage(
newChatOptions.personalDetails.length + newChatOptions.recentReports.length !== 0,
Boolean(newChatOptions.userToInvite),
searchTerm,
maxParticipantsReached,
);
const isOptionsDataReady = ReportUtils.isReportDataReady() && OptionsListUtils.isPersonalDetailsReady(props.personalDetails);

const sections = getSections(maxParticipantsReached);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @allroundexperts
These are the original implementation.
Whenever changing participants in props (run the toggleOption), this component is re-rendered and changes the sections whenever re-rendered.


useEffect(() => {
const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
props.reports,
props.personalDetails,
props.betas,
searchTerm,
props.participants,
CONST.EXPENSIFY_EMAILS,
);
}
setNewChatOptions({
recentReports,
personalDetails,
userToInvite,
});
}, [props.betas, props.reports, props.participants, props.personalDetails, props.translate, searchTerm]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the original implementation having such thing. Can you please explain why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see the updateOptionsWithSearchTerm function and componentDidUpdate.
This useEffect implementation was for replacing above 2 functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which line exactly are you referring to @allroundexperts? If it's 181 that makes sense to me as we'll trigger updates based on those changes. But let me know if you mean somethign else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was talking about the whole useEffect block. @multijump's explanation makes sense though. It's needed indeed.

Copy link
Contributor Author

@multijump multijump Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@allroundexperts @Julesssss

    componentDidUpdate(prevProps) {
        if (_.isEqual(prevProps.reports, this.props.reports) && _.isEqual(prevProps.personalDetails, this.props.personalDetails)) {
            return;
        }
        this.updateOptionsWithSearchTerm(this.state.searchTerm);
    }

    updateOptionsWithSearchTerm(searchTerm = '') {
        const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
            this.props.reports,
            this.props.personalDetails,
            this.props.betas,
            searchTerm,
            this.props.participants,
            CONST.EXPENSIFY_EMAILS,
        );
        this.setState({
            searchTerm,
            userToInvite,
            recentReports,
            personalDetails,
        });
    }

These are original implementation.
Whenever props changes(more exactly, when change reports and personalDetails in props ), componentDidUpdate function is executed and componentDidUpdate calls updateOptionsWithSearchTerm function.

We can change these functions using useEffect

    useEffect(() => {
        const {recentReports, personalDetails, userToInvite} = OptionsListUtils.getNewChatOptions(
            props.reports,
            props.personalDetails,
            props.betas,
            searchTerm,
            props.participants,
            CONST.EXPENSIFY_EMAILS,
        );
        setNewChatOptions({
            recentReports,
            personalDetails,
            userToInvite,
        });
    }, [props.betas, props.reports, props.participants, props.personalDetails, props.translate, searchTerm]);

Like above.

And in the original component, whenever change searchTerm by user type, calls updateOptionsWithSearchTerm function.
So I added searchTerm to useEffect conditions and it allows we can reduce some lines. :)

Actually, in the original implementation, main functional block was updateOptionsWithSearchTerm and I just added it's body to useEffect.
In the useEffect conditions, props.translate is almost constant and if user change the lang in other devices, it should be reflected too so I added it.

FYI, I thought it is not good to reuse the callback function in the component.
That's why I defined setSearchTerm function and added searchTerm to useEffect conditions.


return (
<View style={[styles.flex1, styles.w100, props.participants.length > 0 ? props.safeAreaPaddingBottomStyle : {}]}>
<OptionsSelector
canSelectMultipleOptions
sections={sections}
selectedOptions={props.participants}
value={searchTerm}
onSelectRow={toggleOption}
onChangeText={setSearchTerm}
headerMessage={headerMessage}
boldStyle
shouldShowConfirmButton
confirmButtonText={props.translate('common.next')}
onConfirmSelection={finalizeParticipants}
textInputLabel={props.translate('optionsSelector.nameEmailOrPhoneNumber')}
safeAreaPaddingBottomStyle={props.safeAreaPaddingBottomStyle}
shouldShowOptions={isOptionsDataReady}
/>
</View>
);
}

MoneyRequestParticipantsSplitSelector.propTypes = propTypes;
MoneyRequestParticipantsSplitSelector.defaultProps = defaultProps;
MoneyRequestParticipantsSplitSelector.displayName = 'MoneyRequestParticipantsSplitSelector';

export default compose(
withLocalize,
Expand Down