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.

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

Tweak a localization to remove any assumption on "+" localization #19473

Merged
merged 4 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -54,31 +54,35 @@ struct StatsSegmentedControlData {
}

var differenceLabel: String {
// We want to show something like "+1.2K (5%)" if we have a percentage difference and "1.2K" if we don't.
// Because localized strings need to be strings literal, we cannot embed any conditional logic in the `localizedString...` call.
// We therefore need to generate different string literals based on the state.
let differenceSign = difference < 0 ? "" : "+"
// We want to show something like "+10.2K (+5%)" if we have a percentage difference and "1.2K" if we don't.
//
// Negative cases automatically appear with a negative sign "-10.2K (-5%)" by using `abbreviatedString()`.
// `abbreviatedString()` also handles formatting big numbers, i.e. 10,200 will become 10.2K.
let formatter = NumberFormatter()
formatter.locale = .current
let plusSign = difference <= 0 ? "" : "\(formatter.plusSign ?? "")"

if differencePercent != 0 {
let stringFormat = NSLocalizedString(
"insights.visitorsLineChartCell.differenceLabelWithPercentage",
value: "%@%@ (%@%%)",
comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%)"
value: "%1$@%2$@ (%3$@%%)",
comment: "Text for the Insights Overview stat difference label. Shows the change from the previous period, including the percentage value. E.g.: +12.3K (5%). %1$@ is the placeholder for the change sign ('-', '+', or none). %2$@ is the placeholder for the change numerical value. %3$@ is the placeholder for the change percentage value, excluding the % sign."
)
return String.localizedStringWithFormat(
stringFormat,
differenceSign,
plusSign,
difference.abbreviatedString(),
differencePercent.abbreviatedString()
)
} else {
let stringFormat = NSLocalizedString(
"insights.visitorsLineChartCell.differenceLabelWithoutPercentage",
value: "%@%@",
comment: "Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K"
value: "%1$@%2$@",
comment: "Text for the Insights Overview stat difference label. Shows the change from the previous period. E.g.: +12.3K. %1$@ is the placeholder for the change sign ('-', '+', or none). %2$@ is the placeholder for the change numerical value."
)
return String.localizedStringWithFormat(
stringFormat,
differenceSign,
plusSign,
difference.abbreviatedString()
)
Comment on lines 78 to 87
Copy link
Contributor Author

@mokagio mokagio Oct 18, 2022

Choose a reason for hiding this comment

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

I based this off @staskus suggestion but I decided to use a localized string in the case without the percentage as well, so that translators can apply the appropriate RTL.

I'm not actually sure whether that's necessary, though.

Maybe we should lean into NumberFormatter more for this? There's definitely rooms for improvement and internal refactors. The new tests added in this diff will help us with that in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix. 👍 I think the solution makes sense. We improved it by using numbered positions, localized plus signs, and most importantly, having localized strings static and not dynamic.

}
Expand Down
4 changes: 2 additions & 2 deletions WordPress/Resources/en.lproj/Localizable.strings
Original file line number Diff line number Diff line change
Expand Up @@ -3984,10 +3984,10 @@
/* Title displayed on the feature introduction view that announces the updated Stats Insight screen. */
"Insights update" = "Insights update";

/* Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K */
/* Text for the Insights Overview stat difference label. Shows the change from the previous period. E.g.: +12.3K. %1$@ is the placeholder for the change sign ('-', '+', or none). %2$@ is the placeholder for the change numerical value. */
"insights.visitorsLineChartCell.differenceLabelWithoutPercentage" = "%1$@%2$@";

/* Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%) */
/* Text for the Insights Overview stat difference label. Shows the change from the previous period, including the percentage value. E.g.: +12.3K (5%). %1$@ is the placeholder for the change sign ('-', '+', or none). %2$@ is the placeholder for the change numerical value. %3$@ is the placeholder for the change percentage value, excluding the % sign. */
"insights.visitorsLineChartCell.differenceLabelWithPercentage" = "%1$@%2$@ (%3$@%%)";

/* Button label to install a plugin
Expand Down
8 changes: 6 additions & 2 deletions WordPress/WordPress.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@
3FEC241525D73E8B007AFE63 /* ConfettiView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3FEC241425D73E8B007AFE63 /* ConfettiView.swift */; };
3FF1A853242D5FCB00373F5D /* WPTabBarController+ReaderTabNavigation.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3FF1A852242D5FCB00373F5D /* WPTabBarController+ReaderTabNavigation.swift */; };
3FFA5ED22876152E00830E28 /* JetpackButton.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3FFA5ED12876152E00830E28 /* JetpackButton.swift */; };
3FFE3C0828FE00D10021BB96 /* StatsSegmentedControlDataTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3FFE3C0728FE00D10021BB96 /* StatsSegmentedControlDataTests.swift */; };
400199AB222590E100EB0906 /* AllTimeStatsRecordValueTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 400199AA222590E100EB0906 /* AllTimeStatsRecordValueTests.swift */; };
400199AD22259FF300EB0906 /* AnnualAndMostPopularTimeStatsRecordValueTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 400199AC22259FF300EB0906 /* AnnualAndMostPopularTimeStatsRecordValueTests.swift */; };
400A2C772217A8A0000A8A59 /* VisitsSummaryStatsRecordValue+CoreDataClass.swift in Sources */ = {isa = PBXBuildFile; fileRef = 400A2C752217A8A0000A8A59 /* VisitsSummaryStatsRecordValue+CoreDataClass.swift */; };
Expand Down Expand Up @@ -6091,6 +6092,7 @@
3FEC241425D73E8B007AFE63 /* ConfettiView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ConfettiView.swift; sourceTree = "<group>"; };
3FF1A852242D5FCB00373F5D /* WPTabBarController+ReaderTabNavigation.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WPTabBarController+ReaderTabNavigation.swift"; sourceTree = "<group>"; };
3FFA5ED12876152E00830E28 /* JetpackButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackButton.swift; sourceTree = "<group>"; };
3FFE3C0728FE00D10021BB96 /* StatsSegmentedControlDataTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsSegmentedControlDataTests.swift; sourceTree = "<group>"; };
400199AA222590E100EB0906 /* AllTimeStatsRecordValueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AllTimeStatsRecordValueTests.swift; sourceTree = "<group>"; };
400199AC22259FF300EB0906 /* AnnualAndMostPopularTimeStatsRecordValueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnnualAndMostPopularTimeStatsRecordValueTests.swift; sourceTree = "<group>"; };
400A2C752217A8A0000A8A59 /* VisitsSummaryStatsRecordValue+CoreDataClass.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "VisitsSummaryStatsRecordValue+CoreDataClass.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -10957,9 +10959,12 @@
40F50B7F221310D400CBBB73 /* FollowersStatsRecordValueTests.swift */,
40E7FEC72211EEC00032834E /* LastPostStatsRecordValueTests.swift */,
40EE948122132F5800CD264F /* PublicizeConectionStatsRecordValueTests.swift */,
938466B82683CA0E00A538DC /* ReferrerDetailsViewModelTests.swift */,
400A2C922217B463000A8A59 /* ReferrerStatsRecordValueTests.swift */,
40C403ED2215CE9500E8C894 /* SearchResultsStatsRecordValueTests.swift */,
3FDDFE9527C8178C00606933 /* SiteStatsInformationTests.swift */,
40E7FEC42211DF790032834E /* StatsRecordTests.swift */,
3FFE3C0728FE00D10021BB96 /* StatsSegmentedControlDataTests.swift */,
40F50B81221310F000CBBB73 /* StatsTestCase.swift */,
931215E0267DE1C0008C3B69 /* StatsTotalRowDataTests.swift */,
4054F4632214F94D00D261AB /* StreakStatsRecordValueTests.swift */,
Expand All @@ -10969,8 +10974,6 @@
40C403F72215D88100E8C894 /* TopViewedStatsTests.swift */,
400A2C942217B68D000A8A59 /* TopViewedVideoStatsRecordValueTests.swift */,
400A2C962217B883000A8A59 /* VisitsSummaryStatsRecordValueTests.swift */,
938466B82683CA0E00A538DC /* ReferrerDetailsViewModelTests.swift */,
3FDDFE9527C8178C00606933 /* SiteStatsInformationTests.swift */,
);
name = Stats;
sourceTree = "<group>";
Expand Down Expand Up @@ -22204,6 +22207,7 @@
C738CB0F28626466001BE107 /* QRLoginScanningCoordinatorTests.swift in Sources */,
8B6214E627B1B446001DF7B6 /* BlogDashboardServiceTests.swift in Sources */,
C856749A243F4292001A995E /* TenorMockDataHelper.swift in Sources */,
3FFE3C0828FE00D10021BB96 /* StatsSegmentedControlDataTests.swift in Sources */,
D81C2F5820F86CEA002AE1F1 /* NetworkStatus.swift in Sources */,
E1C545801C6C79BB001CEB0E /* MediaSettingsTests.swift in Sources */,
C3439B5F27FE3A3C0058DA55 /* SiteCreationWizardLauncherTests.swift in Sources */,
Expand Down
48 changes: 48 additions & 0 deletions WordPress/WordPressTest/StatsSegmentedControlDataTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import Nimble
import XCTest
@testable import WordPress

class StatsSegmentedControlDataTests: XCTestCase {

func testDifferenceLabel() {
expect(StatsSegmentedControlData.fixture(difference: -12_345, differencePercent: -1).differenceLabel)
== "-12.3K (-1%)"
expect(StatsSegmentedControlData.fixture(difference: -12_345, differencePercent: 0).differenceLabel)
== "-12.3K"
expect(StatsSegmentedControlData.fixture(difference: -12_345, differencePercent: 1).differenceLabel)
== "-12.3K (1%)"
expect(StatsSegmentedControlData.fixture(difference: 0, differencePercent: -1).differenceLabel)
== "0 (-1%)"
expect(StatsSegmentedControlData.fixture(difference: 0, differencePercent: 0).differenceLabel)
== "0"
expect(StatsSegmentedControlData.fixture(difference: 0, differencePercent: 1).differenceLabel)
== "0 (1%)"
expect(StatsSegmentedControlData.fixture(difference: 12_345, differencePercent: -1).differenceLabel)
== "+12.3K (-1%)"
expect(StatsSegmentedControlData.fixture(difference: 12_345, differencePercent: 0).differenceLabel)
== "+12.3K"
expect(StatsSegmentedControlData.fixture(difference: 12_345, differencePercent: 1).differenceLabel)
== "+12.3K (1%)"
}
}

extension StatsSegmentedControlData {

static func fixture(
segmentTitle: String = "title",
segmentData: Int = 0,
segmentPrevData: Int = 1,
difference: Int = 2,
differenceText: String = "text",
differencePercent: Int = 3
) -> StatsSegmentedControlData {
StatsSegmentedControlData(
segmentTitle: segmentTitle,
segmentData: segmentData,
segmentPrevData: segmentPrevData,
difference: difference,
differenceText: differenceText,
differencePercent: differencePercent
)
}
}