Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[MBL-2120] PLOT - Fixing issue when device locate is ES #2297

Merged
merged 5 commits into from
Feb 20, 2025

Conversation

jovaniks
Copy link
Contributor

📲 What

This PR addresses the currency formatting issue in the Checkout Screen and Manage Pledge screens by using amountFormattedInProjectNativeCurrency for correctly formatted amounts and mapping amountAsFloat to a new string-based field for accurate display in attributed text.

🤔 Why

  • The backend returns values with decimals expressed using a comma (,) instead of a dot (.) for some locales, causing incorrect currency formatting.
  • amountAsFloat is inconsistent across different regional settings, leading to UI issues when displaying amounts.
  • The Manage Pledge screen requires an attributed string format to display the currency symbol and cents correctly, which amountAsFloat alone cannot handle.

🛠 How

  • amountAsFloat is mapped to a new field: PledgePaymentIncrementAmount.amountStringValue (String type).
  • Checkout Screen: Now uses amountFormattedInProjectNativeCurrency to ensure the correct format without additional parsing.
  • Manage Pledge Screen:
    • Created a new function attributedCurrency to handle attributed string formatting for currency values.
    • Added parseAmountComponents to extract integer, decimal separator, and decimal part from amountStringValue for proper UI display.

👀 See

Before 🐛 After 🦋
image image
image

✅ Acceptance criteria

  • Ensure Checkout Screen correctly displays formatted values.
  • Verify Manage Pledge screen displays amounts using attributed strings with correct font sizes.

⏰ TODO

  • Consider refactoring all currency formatting to rely on server-formatted values in the future.

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

I have a question about the expected server behavior before I can continue reviewing this code.

/// - Returns: An `NSAttributedString` where the currency symbol and decimals are displayed in a smaller font.
/// If the amount cannot be parsed, it returns the amount as a plain attributed string.
public static func attributedCurrency(
amountString: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question. amountString will be set to the amountAsFloat sent from the server, correct? Why would amountAsFloat include any regional formatting, like comma separators? My assumption would be that the server should be sending a 'plain' string like "2499.75", not "2,499.75".

@lukert33 can you clarify the server's behavior 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.

That's correct, amountString takes its value from amountAsFloat. I feel like amountAsFloat represents a money format without currency symbols.

GraphQL response:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, that's not what I would have expected. Surprising.

Copy link
Contributor

@scottkicks scottkicks Feb 19, 2025

Choose a reason for hiding this comment

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

Can you link the ticket that will address cleaning these format helpers up once the backend is updated to give us the correct formatted string as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing what's changed in these snapshots 🤔

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Good work. Looking forward to having the server handle this for us so we dont have to do so much formatting client side

Copy link
Contributor Author

@jovaniks jovaniks left a comment

Choose a reason for hiding this comment

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

PLOT UI now fully uses amountFormattedInProjectNativeCurrency for amounts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snapshots updated since the increments amounts are using a plain text instead of a attributed text

@@ -84,38 +84,12 @@ public struct PLOTPaymentScheduleItem: Equatable {
timeStyle: .none
)

self.amountString = increment.amount.amountFormattedInProjectNativeCurrency
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Set amountFormattedInProjectNativeCurrency directly to have a plain text in the UILabel

with: increments,
project: project
)
.observeValues { [weak self] increments in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed project since it is not more necessary

@@ -100,7 +100,7 @@ final class PledgeOverTimePaymentScheduleViewController: UIViewController {
itemView.configure(
with: item.dateString,
badgeTitle: item.stateLabel,
amountAttributedText: item.amountAttributedText,
amountString: item.amountString,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use a string instead of a attributedText due the UI is showing a plain text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Snapshots updated due the amount labels are using a plain text instead of a attributed text with currency symbols and decimals with smaller size and offset

@@ -1,6 +1,5 @@
fragment PaymentIncrementFragment on PaymentIncrement {
amount {
amountAsFloat
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need amountAsFloat anymore

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for reworking this.

There are two follow-up tickets from this PR:

https://kickstarter.atlassian.net/browse/PAY-3468
https://kickstarter.atlassian.net/browse/MBL-2123

@jovaniks jovaniks merged commit ebb2c32 into main Feb 20, 2025
5 checks passed
@jovaniks jovaniks deleted the jluna/MBL-2120/plot-decimal-issue branch February 20, 2025 16:37
stevestreza-ksr pushed a commit that referenced this pull request Feb 20, 2025
* Fixing issue when device locate is ES

* Fixing test

* Removing `amountAsFloat` and only use `amountFormattedInProjectNativeCurrency` to represents the amounts in all of the PLOT stuff related

* Remove unused Project reference
stevestreza-ksr added a commit that referenced this pull request Feb 25, 2025
* marketing version to 5.23.0

* [MBL-2120] PLOT - Fixing issue when device locate is ES (#2297)

* Fixing issue when device locate is ES

* Fixing test

* Removing `amountAsFloat` and only use `amountFormattedInProjectNativeCurrency` to represents the amounts in all of the PLOT stuff related

* Remove unused Project reference

---------

Co-authored-by: jovaniks <j.luna.c@kickstarter.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants