-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Make number suffixes translatable through an injected class #10818
Make number suffixes translatable through an injected class #10818
Conversation
Generated by 🚫 dangerJS |
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @planarvoid, nice improvement! ;)
I've found one possible bug, but it looks good otherwise.
@@ -119,7 +120,7 @@ constructor( | |||
icon = icon, | |||
iconUrl = if (icon == null) group.icon else null, | |||
text = group.name, | |||
value = group.total?.toFormattedString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this would be evaluated to "null" before the change, but is evaluated to "0" now. Is this change intentional? Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This was happening in multiple places so I fixed it in 9a6d791
We now have 3 methods (for all number formats):
- One with nullable parameter and no default value -> returns nullable result
- One with nullable parameter and mandatory default value -> returns non-null result
- One with non-null parameter -> returns non-null result
I've added tests for all the new methods in 0c12f68 and f9ffab0
@malinajirka thanks for the check! I've fixed the issue and added tests. Let me know if the solution is sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Fixes #10753
This PR removes the
toFormattedString
extension function and replaces it with a call on an injectedStatsUtils
object. The extension function doesn't make sense since we have to use other injected fields to build the formatted string (resource provider and locale manager).To test:
PR submission checklist:
I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.