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 13 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
229 changes: 102 additions & 127 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, {useCallback, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
Expand Down Expand Up @@ -60,183 +60,158 @@ const defaultProps = {
safeAreaPaddingBottomStyle: {},
};

class MoneyRequestParticipantsSplitSelector extends Component {
constructor(props) {
super(props);
function MoneyRequestParticipantsSplitSelector(props) {
const [searchTerm, setSearchTerm] = useState('');
const [newChatOptions, setNewChatOptions] = useState({
recentReports: [],
personalDetails: [],
userToInvite: null,
});

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);
}
const maxParticipantsReached = props.participants.length === CONST.REPORT.MAXIMUM_PARTICIPANTS;

/**
* Returns the sections needed for the OptionsSelector
*
* @param {Boolean} maxParticipantsReached
* @returns {Array}
*/
getSections(maxParticipantsReached) {
const sections = [];
const sections = 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;
// eslint-disable-next-line react-hooks/exhaustive-deps -- props does not need to be a dependency as it will always exist
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 disabling this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image I got this warning when I checked lint. So I checked the useMemo and useCallback functions in other components and I noticed they did same comment. That's why I disabled 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 seems like we're no longer disabling the check, but I think it dows make sense, right? We shouldn't update after ANY prop is changed, just specific ones?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct @Julesssss. We've added into dependency all the properties that should trigger a re-calculation of values in the function.

}, [maxParticipantsReached, newChatOptions, props.participants, props.translate]);

/**
* 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 = useCallback(
(option) => {
const isOptionInList = _.some(props.participants, (selectedOption) => selectedOption.accountID === option.accountID);

let newSelectedOptions;
let newSelectedOptions;

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

this.props.onAddParticipants(newSelectedOptions);
props.onAddParticipants(newSelectedOptions);

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

setNewChatOptions({
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 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>
});
if (!isOptionInList) {
setSearchTerm('');
}
},
// eslint-disable-next-line react-hooks/exhaustive-deps -- props does not need to be a dependency as it will always exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question

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 got a warning when I checked lint. So I checked the useMemo and useCallback functions in other components and I noticed they did same comment. That's why I disabled it.

[searchTerm, props.participants, props.onAddParticipants, props.reports, props.personalDetails, props.betas, setNewChatOptions, setSearchTerm],
);

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

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={props.onStepComplete}
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