From 7e7210dd8f382be49e3e618df05e7f9c09d1ac8b Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Mon, 17 Oct 2022 22:02:58 +1100 Subject: [PATCH 1/4] Tweak a localization to remove any assumption on "+" localization See discussion at https://github.com/wordpress-mobile/WordPress-iOS/pull/19472#discussion_r996700821 --- .../ViewsVisitorsLineChartCell.swift | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift b/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift index 77222c7662e4..78c50d7d535d 100644 --- a/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift +++ b/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift @@ -57,30 +57,45 @@ struct StatsSegmentedControlData { // 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 ? "" : "+" - if differencePercent != 0 { - let stringFormat = NSLocalizedString( - "insights.visitorsLineChartCell.differenceLabelWithPercentage", - value: "%@%@ (%@%%)", + switch (difference > 0, differencePercent != 0) { + case (true, true): // E.g.: +1.2k (5%) + stringFormat = NSLocalizedString( + "insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage", + value: "+%@ (%@%%)", comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%)" ) return String.localizedStringWithFormat( stringFormat, - differenceSign, difference.abbreviatedString(), differencePercent.abbreviatedString() ) - } else { - let stringFormat = NSLocalizedString( - "insights.visitorsLineChartCell.differenceLabelWithoutPercentage", - value: "%@%@", + case (true, false): // E.g.: +1.2k + // We cannot assume every locale would translate an English string like "+1.2k" in the same way. + // So, even though we have only a "+" prefix, we ought to make this string localized. + stringFormat = NSLocalizedString( + "insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage", + value: "+%@", comment: "Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K" ) return String.localizedStringWithFormat( stringFormat, - differenceSign, difference.abbreviatedString() ) + case (false, true): // E.g. 1.2k (5%) + stringFormat = NSLocalizedString( + "insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage", + value: "%@ (%@%%)", + comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value, when the change is 0 or less. Example: 99.9K (5%)" + ) + return String.localizedStringWithFormat( + stringFormat, + difference.abbreviatedString(), + differencePercent.abbreviatedString() + ) + break + case (false, false): // E.g.: 1.2k + // There's no + sign nor percentage value here, we don't need to add any localization treatment. + return difference.abbreviatedString() } } From daec387a70218ba91c4799c1cb2e54193dc44329 Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Mon, 17 Oct 2022 22:09:28 +1100 Subject: [PATCH 2/4] Update strings for localization --- WordPress/Resources/en.lproj/Localizable.strings | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/WordPress/Resources/en.lproj/Localizable.strings b/WordPress/Resources/en.lproj/Localizable.strings index d54bdf825659..6e11246c0392 100644 --- a/WordPress/Resources/en.lproj/Localizable.strings +++ b/WordPress/Resources/en.lproj/Localizable.strings @@ -3984,11 +3984,14 @@ /* 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, including percentage value, when the change is 0 or less. Example: 99.9K (5%) */ +"insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage" = "%1$@ (%2$@%%)"; + /* Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K */ -"insights.visitorsLineChartCell.differenceLabelWithoutPercentage" = "%1$@%2$@"; +"insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage" = "+%@"; /* Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%) */ -"insights.visitorsLineChartCell.differenceLabelWithPercentage" = "%1$@%2$@ (%3$@%%)"; +"insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage" = "+%1$@ (%2$@%%)"; /* Button label to install a plugin Confirmation button displayd in alert displayed when user installs their first plugin. */ From cd7b1311c47ddf1ca3668f7383e291dad7ccba13 Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Tue, 18 Oct 2022 12:19:01 +1100 Subject: [PATCH 3/4] Refine `differenceLabel` implementation and add tests --- .../ViewsVisitorsLineChartCell.swift | 51 ++++++++----------- WordPress/WordPress.xcodeproj/project.pbxproj | 8 ++- .../StatsSegmentedControlDataTests.swift | 48 +++++++++++++++++ 3 files changed, 74 insertions(+), 33 deletions(-) create mode 100644 WordPress/WordPressTest/StatsSegmentedControlDataTests.swift diff --git a/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift b/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift index 78c50d7d535d..0769cc600175 100644 --- a/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift +++ b/WordPress/Classes/ViewRelated/Stats/Insights/ViewsVisitors/ViewsVisitorsLineChartCell.swift @@ -54,48 +54,37 @@ 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. - switch (difference > 0, differencePercent != 0) { - case (true, true): // E.g.: +1.2k (5%) - stringFormat = NSLocalizedString( - "insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage", - value: "+%@ (%@%%)", - comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%)" + // 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: "%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, + plusSign, difference.abbreviatedString(), differencePercent.abbreviatedString() ) - case (true, false): // E.g.: +1.2k - // We cannot assume every locale would translate an English string like "+1.2k" in the same way. - // So, even though we have only a "+" prefix, we ought to make this string localized. - stringFormat = NSLocalizedString( - "insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage", - value: "+%@", - comment: "Difference label for Insights Overview stat, indicating change from previous period. Example: +99.9K" + } else { + let stringFormat = NSLocalizedString( + "insights.visitorsLineChartCell.differenceLabelWithoutPercentage", + 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, + plusSign, difference.abbreviatedString() ) - case (false, true): // E.g. 1.2k (5%) - stringFormat = NSLocalizedString( - "insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage", - value: "%@ (%@%%)", - comment: "Difference label for Insights Overview stat, indicating change from previous period, including percentage value, when the change is 0 or less. Example: 99.9K (5%)" - ) - return String.localizedStringWithFormat( - stringFormat, - difference.abbreviatedString(), - differencePercent.abbreviatedString() - ) - break - case (false, false): // E.g.: 1.2k - // There's no + sign nor percentage value here, we don't need to add any localization treatment. - return difference.abbreviatedString() } } diff --git a/WordPress/WordPress.xcodeproj/project.pbxproj b/WordPress/WordPress.xcodeproj/project.pbxproj index 70ace6b4063d..bd60643f13aa 100644 --- a/WordPress/WordPress.xcodeproj/project.pbxproj +++ b/WordPress/WordPress.xcodeproj/project.pbxproj @@ -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 */; }; @@ -6091,6 +6092,7 @@ 3FEC241425D73E8B007AFE63 /* ConfettiView.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ConfettiView.swift; sourceTree = ""; }; 3FF1A852242D5FCB00373F5D /* WPTabBarController+ReaderTabNavigation.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WPTabBarController+ReaderTabNavigation.swift"; sourceTree = ""; }; 3FFA5ED12876152E00830E28 /* JetpackButton.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JetpackButton.swift; sourceTree = ""; }; + 3FFE3C0728FE00D10021BB96 /* StatsSegmentedControlDataTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsSegmentedControlDataTests.swift; sourceTree = ""; }; 400199AA222590E100EB0906 /* AllTimeStatsRecordValueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AllTimeStatsRecordValueTests.swift; sourceTree = ""; }; 400199AC22259FF300EB0906 /* AnnualAndMostPopularTimeStatsRecordValueTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AnnualAndMostPopularTimeStatsRecordValueTests.swift; sourceTree = ""; }; 400A2C752217A8A0000A8A59 /* VisitsSummaryStatsRecordValue+CoreDataClass.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "VisitsSummaryStatsRecordValue+CoreDataClass.swift"; sourceTree = ""; }; @@ -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 */, @@ -10969,8 +10974,6 @@ 40C403F72215D88100E8C894 /* TopViewedStatsTests.swift */, 400A2C942217B68D000A8A59 /* TopViewedVideoStatsRecordValueTests.swift */, 400A2C962217B883000A8A59 /* VisitsSummaryStatsRecordValueTests.swift */, - 938466B82683CA0E00A538DC /* ReferrerDetailsViewModelTests.swift */, - 3FDDFE9527C8178C00606933 /* SiteStatsInformationTests.swift */, ); name = Stats; sourceTree = ""; @@ -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 */, diff --git a/WordPress/WordPressTest/StatsSegmentedControlDataTests.swift b/WordPress/WordPressTest/StatsSegmentedControlDataTests.swift new file mode 100644 index 000000000000..f0914ec1f0e5 --- /dev/null +++ b/WordPress/WordPressTest/StatsSegmentedControlDataTests.swift @@ -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 + ) + } +} From ede3b8e55bb2b3073e43125b5cae601ae1f4982b Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Tue, 18 Oct 2022 12:58:06 +1100 Subject: [PATCH 4/4] Update strings for localization --- WordPress/Resources/en.lproj/Localizable.strings | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/WordPress/Resources/en.lproj/Localizable.strings b/WordPress/Resources/en.lproj/Localizable.strings index 6e11246c0392..4fe4f3f5070f 100644 --- a/WordPress/Resources/en.lproj/Localizable.strings +++ b/WordPress/Resources/en.lproj/Localizable.strings @@ -3984,14 +3984,11 @@ /* 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, including percentage value, when the change is 0 or less. Example: 99.9K (5%) */ -"insights.visitorsLineChartCell.differenceLabelNotPosiviteWithPercentage" = "%1$@ (%2$@%%)"; +/* 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. Example: +99.9K */ -"insights.visitorsLineChartCell.differenceLabelPosiviteWithoutPercentage" = "+%@"; - -/* Difference label for Insights Overview stat, indicating change from previous period, including percentage value. Example: +99.9K (5%) */ -"insights.visitorsLineChartCell.differenceLabelPosiviteWithPercentage" = "+%1$@ (%2$@%%)"; +/* 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 Confirmation button displayd in alert displayed when user installs their first plugin. */