Widgets: Data formatting #7708
Widgets: Data formatting #7708
Conversation
You can test the changes from this Pull Request by:
|
| /// Redacted entry with sample data. | ||
| /// | ||
| func placeholder(in context: Context) -> StoreInfoEntry { | ||
| let dependencies = Self.fetchDependencies() | ||
| return StoreInfoEntry.data(.init(range: Localization.today, | ||
| name: dependencies?.storeName ?? Localization.myShop, | ||
| revenue: "$132.234", | ||
| revenue: formattedAmountString(for: 132.234, with: dependencies?.storeCurrencySettings), |
| enum CodingKeys: CodingKey { | ||
| case currencyCode | ||
| case currencyPosition | ||
| case groupingSeparator | ||
| case decimalSeparator | ||
| case fractionDigits | ||
| } | ||
|
|
||
| public required init(from decoder: Decoder) throws { | ||
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
| currencyCode = try container.decode(CurrencyCode.self, forKey: .currencyCode) | ||
| currencyPosition = try container.decode(CurrencyPosition.self, forKey: .currencyPosition) | ||
| groupingSeparator = try container.decode(String.self, forKey: .groupingSeparator) | ||
| decimalSeparator = try container.decode(String.self, forKey: .decimalSeparator) | ||
| fractionDigits = try container.decode(Int.self, forKey: .fractionDigits) | ||
| } | ||
|
|
||
| public func encode(to encoder: Encoder) throws { | ||
| var container = encoder.container(keyedBy: CodingKeys.self) | ||
| try container.encode(currencyCode, forKey: .currencyCode) | ||
| try container.encode(currencyPosition, forKey: .currencyPosition) | ||
| try container.encode(groupingSeparator, forKey: .groupingSeparator) | ||
| try container.encode(decimalSeparator, forKey: .decimalSeparator) | ||
| try container.encode(fractionDigits, forKey: .fractionDigits) | ||
| } |
There was a problem hiding this comment.
Since we have a custom encoding here, maybe its worth noting that it was added for the widget extension. So future devs will be more informed.
There was a problem hiding this comment.
It's actually not custom, but required because of @Published property preventing auto-synthesized conformance.
Agree about a comment, added in 9abe891
| } | ||
| } | ||
|
|
||
| private extension StoreInfoProvider { | ||
|
|
||
| func formattedAmountString(for amountValue: Decimal, with currencySettings: CurrencySettings?) -> String { |
There was a problem hiding this comment.
I think these could be static, right? Since they don't access self at all.
There was a problem hiding this comment.
Yes, updated in 37cca09
| let minimumFractionDigits = floor(conversionRate * 100.0) == conversionRate * 100.0 ? 0 : 1 | ||
| numberFormatter.minimumFractionDigits = minimumFractionDigits | ||
| return numberFormatter.string(from: conversionRate as NSNumber) ?? Constants.valuePlaceholderText |
There was a problem hiding this comment.
I think there is a bit of an edge case here or in the conversion formula as because 100% is showing as 10.000%
In retrospective, I should have added unit tests to these types 🙈
There was a problem hiding this comment.
Yeah, very obvious error, converted rate value to percentage twice 😄
Fixed in 72e118c.
|
@Ecarrion thanks for useful feedback and catching conversion bug! |
Closes: #7565
Depends on #7707 (not logic, it's just to prevent merge conflicts in same files).
Description
We need store currency setting to correctly display amount field. This PR also fixes conversion field from overflow over 100%.
How
CurrencySettingsand shares it between app and extension.Testing Steps
wp-adminit's in WooCommerce -> Settings -> Currency Options.Screenshots
RELEASE-NOTES.txtif necessary.