Skip to content

Conversation

@iwiznia
Copy link
Contributor

@iwiznia iwiznia commented May 18, 2021

Details

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/164533

Tests

Request Money

  • Click the + icon and select Request Money
  • Enter $50 on the IOUAmount page and click next
  • Observe that the header of the IOUParticipants page reads Request $50
    image
  • Select a participant to move on to the IOUConfirm page
  • Observe that the header & confirmation button both read Request $50
    image

Split Bill

  • Click the + icon and select Split Bill
  • Enter $50 on the IOUAmount page and click next
  • Observe that the header of the IOUParticipants page reads Split $50
    image
  • Select a couple of participants and click next
  • Observe that the header of the IOUConfirm page also reads Split $50
    image

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@iwiznia iwiznia requested a review from Julesssss May 18, 2021 22:53
@iwiznia iwiznia self-assigned this May 18, 2021
@iwiznia iwiznia requested a review from a team as a code owner May 18, 2021 22:53
@MelvinBot MelvinBot requested review from Beamanator and removed request for a team May 18, 2021 22:53
@Julesssss
Copy link
Contributor

@iwiznia FYI conflicts

Comment on lines 227 to 234
const buttonText = this.props.translate(
this.props.hasMultipleParticipants ? 'common.split' : 'iou.request', {
amount: this.props.numberFormat(
this.props.iouAmount,
{style: 'currency', currency: this.props.selectedCurrency},
),
},
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We've got a condition, translation, and number format all in one chunk. Maybe separating out amount would simplify this statement a bit.

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 don't see why adding more variable is simpler, the amount of code is the same.

Copy link
Contributor

@Beamanator Beamanator May 19, 2021

Choose a reason for hiding this comment

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

Looking at it like this, I'm not sure it offers too much of an improvement 🤷

const phrase = this.props.hasMultipleParticipants ? 'common.split' : 'iou.request';
const variables = {
    amount: this.props.numberFormat(
        this.props.iouAmount,
        {style: 'currency', currency: this.props.selectedCurrency},
    ),
};
const buttonText = this.props.translate(phrase, variables);

to: 'To',
optional: 'Optional',
split: 'Split',
split: ({amount}) => `Split ${amount}`,
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 it shouldn't be common anymore, as it must have an amount. It should be listed beneath iou.request IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that it has an amount does not make a difference. We are using this in 3 places, so it is pretty common to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see iou.request used in the same places as common.split - both in IOUConfirmationList.js and IOUModal.js.

My opinion is that adding an ${amount} in the text makes it very iou-related so I'd agree with Jules.

Comment on lines +131 to +137
amount: this.props.numberFormat(
this.state.amount, {
style: 'currency',
currency: this.state.selectedCurrency,
},
),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Separating this into two steps would be a lot simpler.

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Requested a few changes

@iwiznia
Copy link
Contributor Author

iwiznia commented May 19, 2021

Updated

@iwiznia
Copy link
Contributor Author

iwiznia commented May 19, 2021

Updated

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Tested and works well on Desktop & iOS 👍

@Julesssss Julesssss merged commit a84d9ab into main May 20, 2021
@Julesssss Julesssss deleted the ionatan_bad_amounts branch May 20, 2021 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants