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

Update Who Paid section when can modify participants #21945

Merged
merged 4 commits into from
Jul 6, 2023
Merged

Update Who Paid section when can modify participants #21945

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a376a1
Update Who Paid section when can modify participants
ginsuma Jul 3, 2023
b159a1d
Fix lint
ginsuma Jul 3, 2023
62253ce
Add shouldDisableWhoPaidSection
ginsuma Jul 3, 2023
64ff1d1
Fix lint
ginsuma Jul 5, 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
16 changes: 14 additions & 2 deletions src/components/MoneyRequestConfirmationList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ function MoneyRequestConfirmationList(props) {

const selectedParticipants = useMemo(() => _.filter(props.participants, (participant) => participant.selected), [props.participants]);
const payeePersonalDetails = useMemo(() => props.payeePersonalDetails || props.currentUserPersonalDetails, [props.payeePersonalDetails, props.currentUserPersonalDetails]);
const canModifyParticipants = !props.isReadOnly && props.canModifyParticipants && props.hasMultipleParticipants;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we have a better naming here canModifyParticipants off as we are not modifying who paid anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How's about shouldDisableWhoPaidSection?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okayish make the changes, 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.

I made it, thank you.

const shouldDisableWhoPaidSection = canModifyParticipants;

const optionSelectorSections = useMemo(() => {
const sections = [];
Expand All @@ -152,6 +154,7 @@ function MoneyRequestConfirmationList(props) {
data: [formattedPayeeOption],
shouldShow: true,
indexOffset: 0,
isDisabled: shouldDisableWhoPaidSection,
},
{
title: translate('moneyRequestConfirmationList.whoWasThere'),
Expand All @@ -169,7 +172,17 @@ function MoneyRequestConfirmationList(props) {
});
}
return sections;
}, [selectedParticipants, getParticipantsWithAmount, props.hasMultipleParticipants, props.iouAmount, props.iouCurrencyCode, props.participants, translate, payeePersonalDetails]);
}, [
props.participants,
props.hasMultipleParticipants,
props.iouAmount,
props.iouCurrencyCode,
getParticipantsWithAmount,
selectedParticipants,
payeePersonalDetails,
translate,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we need translate here.

NAB (as it's not added here)

Copy link
Contributor

Choose a reason for hiding this comment

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

It gets flagged as a missing dependency by the linter otherwise (of course we could ignore the linter in this case, but I think it's fine to just include it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

translate is a prop from withLocalize, and it is used in above useMemo.
When user change their language from another tab/window, it should update the title in section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, thanks for calling that out. I was previously under the impression that including translate in the dependency array was in case the function changed, not what the function returns 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. Maybe the below make it clear.

App/src/components/MoneyRequestConfirmationList.js

Lines 102 to 104 in b595101

// Destructure functions from props to pass it as a dependecy to useCallback/useMemo hooks.
// Prop functions pass props itself as a "this" value to the function which means they change every time props change.
const {translate, onSendMoney, onConfirm, onSelectParticipant} = props;

shouldDisableWhoPaidSection,
]);

const selectedOptions = useMemo(() => {
if (!props.hasMultipleParticipants) {
Expand Down Expand Up @@ -228,7 +241,6 @@ function MoneyRequestConfirmationList(props) {
[selectedParticipants, onSendMoney, onConfirm, props.iouType],
);

const canModifyParticipants = !props.isReadOnly && props.canModifyParticipants && props.hasMultipleParticipants;
const formattedAmount = CurrencyUtils.convertToDisplayString(props.iouAmount, props.iouCurrencyCode);

const footerContent = useMemo(() => {
Expand Down