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 4 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
234 changes: 106 additions & 128 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 @@ -59,183 +59,161 @@ 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;
return newSections;
}

updateOptionsWithSearchTerm(searchTerm = '') {
/**
* Removes a selected option from list if already selected. If not already selected add this option to the list.
* @param {Object} option
*/
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.login === option.login);

let newSelectedOptions;

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

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

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
});
props.onAddParticipants(newSelectedOptions)
}

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

/**
* 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.login === option.login);

let newSelectedOptions;

if (isOptionInList) {
newSelectedOptions = _.reject(this.props.participants, (selectedOption) => selectedOption.login === option.login);
} else {
newSelectedOptions = [...this.props.participants, option];
}
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);

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 : '',
};
});
}
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.


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>
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, searchTerm])
Copy link
Member

Choose a reason for hiding this comment

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

Hi @multijump, I think we should re-render if props.translate changes also.

i.e. if the language is changed on device B, device A should re-render.
Does that make sense?

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, @rushatgabhane
Yes, that make sense.
just updated following your instruction.
Thanks.


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