Reader: Fix Post Count Not Appearing #87864

Aurorum · 2024-02-25T21:10:52Z

Proposed Changes

For some reason, formatNumberCompact doesn't seem to work. I'd like to propose getting rid of it for two reasons:

It's only used in the Reader.
We have formatNumber, which as far as I can see achieves the same thing. In fact, it's already used in more recent code in the Reader - that's why the subscription count works, but not the post count!

Testing Instructions

Edit: turns out you need to use a locale that isn't hardcoded in the formatNumberCompact list! For instance, British English.

You can see the difference here: https://wordpress.com/read/feeds/25823

Before:

After:

cc @eoigal, @davemart-in

Aurorum · 2024-02-25T21:30:27Z

packages/components/src/count/index.jsx

@@ -15,7 +15,9 @@ export const Count = ( { count, compact, numberFormat, forwardRef, primary, ...r
 			className={ classnames( 'count', { 'is-primary': primary } ) }
 			{ ...inheritProps }
 		>
-			{ compact ? formatNumberCompact( count ) || numberFormat( count ) : numberFormat( count ) }
+			{ compact
+				? formatNumber( count, getLocaleSlug() ) || numberFormat( count )


The mystery continues 🤔 The Count component also uses its own formatNumber which, as far as I can see, was copied over from the original function. But the only thing that stops it breaking currently is the fallback; it doesn't format properly.

This PR fixes it (I think?) but I wonder if I'm missing something? @jeyip @chriskmnds might you be able to advise from working on #79257? Thanks!

Aurorum · 2024-02-25T21:39:38Z

Ahhhhhh I see - formatNumberCompact attempts to hardcode the locales on WordPress.com, so it automatically breaks for all locales where that isn't possible. In my case, I use British English (en-gb), so that's an example of a locale where that doesn't exist.

formatNumber uses Intl.NumberFormat rather than hardcoding, so it works the locales that weren't hardcoded. I still favour getting rid of formatNumberCompact rather than updating the hardcoded list, partly because it's easy to miss locales (as demonstrated!), but also we can purge ~500 lines of code.

Aurorum · 2024-03-14T08:34:24Z

cc @tyxla - you might be pleased about the chance to clean up some code? 😁

tyxla · 2024-03-15T15:33:42Z

client/reader/stream/reader-tag-sidebar/index.jsx

@@ -1,14 +1,14 @@
 import { Button } from '@automattic/components';
+import formatNumber from '@automattic/components/src/number-formatters/lib/format-number';


I'd generally recommend importing only public API. That means going with one of the following alternatives:

Exporting formatNumber publicly from the package and consuming it from there

Moving formatNumber to another package where it makes the most sense.

Since number formatting has little to do with components, I'd recommend the latter.

Agreed. I've moved it to i18n-utils, where I'd argue it makes the most sense, since this is mostly about ensuring correct number formatting in all languages.

Thanks.

I'd like feedback @Automattic/i18n on this as well. Particularly because we already have a numberFormat utility in the i18n-calypso package and I'm not sure how much we want to duplicate.

The part that could be concerning is the locale. If I understand correctly, we won't consider the user locale from the user profile, but only the browser one, which is often not the right one. I have serious doubts that this is what we want to do; I believe instead we want to use the locale from the user's chosen interface language.

My understanding is that we pass the locale slug specifically to avoid this? For instance, this is me with my WP.com profile in Turkish, and my browser in English.

Do note that in some cases, such as the above one, we're already using the browser for the number on the right but not the one on the left!

Ahhhhh - I see. I think that's an oversight on my end; we should pass the slug, but I didn't update the ones where formattedNumber is used. I'll change that.

Although I do wonder if it might be easier not to require the slug to be passed? translate does it, so it must somehow be possible (though haven't looked into it yet!).

translate() is a different deal. It will use the internal i18n instance that gets updated once the slug changes, while the current formatting function you're working with won't do that - it will just statically use the default slug.

Forgive me as I'm not quite sure I understand how this i18n works, but would it not be possible to just use the default slug like this?

wp-calypso/packages/i18n-utils/src/localize-url.tsx

Lines 21 to 23 in b9ea6f0

function getDefaultLocale(): Locale {

return getLocaleSlug?.() ?? 'en';

}

Would that have unexpected problems?

This leads to the same potential issue with using getLocaleSlug() directly that I pointed out in #87864 (comment).

tyxla · 2024-03-15T15:39:25Z

client/reader/stream/reader-tag-sidebar/index.jsx

@@ -114,13 +115,13 @@ const ReaderTagSidebar = ( {
 				<div className="reader-tag-sidebar-stats">
 					<div className="reader-tag-sidebar-stats__item">
 						<span className="reader-tag-sidebar-stats__count">
-							{ formatNumberCompact( tagStats?.data?.total_posts ) }
+							{ formatNumber( tagStats?.data?.total_posts, localeSlug ) }


I see formatNumber() from @automattic/components and formatNumberCompact() from /lib are quite different and the latter is custom while the former uses Intl.NumberFormat. While I prefer using Intl.NumberFormat() generally, I'm concerned about any unexpected differences or regressions that this change might cause. Have you done a comparison between the two? I think some tests may be necessary to ensure we don't break something.

I've added some tests - see below for my comments on the differences! Only other difference I could find is that with the exact number, there is one less significant number in the digit (ie. 1000 becomes 1K instead of 1.0K).

I've checked the contexts in which this is used, and I personally don't think that's a big deal - especially since formatNumberCompact() doesn't account for all locales, whereas Intl.NumberFormat does.

Would be nice to list all the places where this is affected, since we might want to check them with design folks and ensure that the change is alright.

tyxla · 2024-03-15T15:49:49Z

client/reader/tags/trending-tags.tsx

 import { addLocaleToPathLocaleInFront } from '@automattic/i18n-utils';
+import { getLocaleSlug } from 'i18n-calypso';


There are a few issues with using getLocaleSlug() directly for getting the locale slug:

It's shared between the client and the server so it might cause unexpected issues with translations on the server (and trending tags is SSR-ed as part of the /tags page)

getLocaleSlug() is just a method so if the locale changes, it won't trigger a re-render, which might cause obsolete strings to be displayed.

with that in mind, I'd recommend either:

Using the useTranslate() hook for functional components, it also returns a localeSlug property.

Using the localize() high-order component for class components, it also returns a localeSlug property.

Hat tip to @yuliyan for the extra info on this.

Ah, never knew that! I've made those changes now - let me know if I've misunderstood what you're getting at. :)

Thanks - that change seems alright. The only concern I have is around the inconsistent use of the locale slug - sometimes we'll pass it to formatNumber(), sometimes we won't, and that may lead to inconsistent results when a non-EN locale is currently used for the user's WP.com interface.

tyxla · 2024-03-15T15:50:32Z

client/lib/format-number-compact/test/index.js

@@ -1,195 +1,4 @@
-import formatNumberCompact, { formatNumberMetric } from 'calypso/lib/format-number-compact';
-
-describe( 'formatNumberCompact', () => {


I wonder what would happen if we run these tests against the alternative solution that you're recommending to use 🤔

It looks like there are some differences - the Arabic version, for instance, used to be 1٫2 ألف. It's now ١٫٢ ألف.

I'm inclined to defer to Intl.NumberFormat on this.

I'd want to double-check this though, since it might introduce unexpected regressions in some locales.

On top of the @Automattic/i18n team you already pinged, perhaps we could ask a native speaker? IIRC @alshakero could help, but he's on leave. The i18n team is also on a meetup this week AFAIK, so this might take a bit longer than usual.

Doesn't quite compare to asking a native speaker (!), but this specific one seems OK:

Totally understand the desire to double-check though.

Definitely don't intend to block this work. I just don't feel particularly knowledgeable about this aspect of i18n. The i18n team has historically worked with RTL languages way more, so I hope they will have a better idea.

Aurorum added 2 commits February 25, 2024 20:45

Reader: Fix Post Count not Appearing

cc1d351

Remove formatNumberCompact

ec68c2e

Aurorum requested a review from a team as a code owner February 25, 2024 21:10

github-actions bot added the OSS Citizen label Feb 25, 2024

Aurorum added 2 commits February 25, 2024 21:20

Add slugs

9b5434f

Purge more code

b9ecc70

Aurorum commented Feb 25, 2024

View reviewed changes

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 25, 2024

tyxla reviewed Mar 15, 2024

View reviewed changes

Aurorum added 2 commits March 16, 2024 17:00

Merge remote-tracking branch 'upstream/trunk' into fix/reader-post-count

985b8d3

Address review

a395922

Aurorum requested a review from a team as a code owner March 16, 2024 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: Fix Post Count Not Appearing #87864

Reader: Fix Post Count Not Appearing #87864

Aurorum commented Feb 25, 2024 •

edited

Loading

Aurorum Feb 25, 2024

Aurorum commented Feb 25, 2024

Aurorum commented Mar 14, 2024

tyxla Mar 15, 2024

Aurorum Mar 16, 2024

tyxla Mar 18, 2024

tyxla Mar 18, 2024

Aurorum Mar 18, 2024

Aurorum Mar 18, 2024

Aurorum Mar 18, 2024

tyxla Mar 18, 2024

Aurorum Mar 18, 2024

tyxla Mar 18, 2024

tyxla Mar 15, 2024

Aurorum Mar 16, 2024 •

edited

Loading

tyxla Mar 18, 2024

tyxla Mar 15, 2024

Aurorum Mar 16, 2024

tyxla Mar 18, 2024

tyxla Mar 15, 2024

Aurorum Mar 16, 2024

tyxla Mar 18, 2024

Aurorum Mar 18, 2024

tyxla Mar 18, 2024

		@@ -1,14 +1,14 @@
		import { Button } from '@automattic/components';
		import formatNumber from '@automattic/components/src/number-formatters/lib/format-number';

	function getDefaultLocale(): Locale {
	return getLocaleSlug?.() ?? 'en';
	}

		import { addLocaleToPathLocaleInFront } from '@automattic/i18n-utils';
		import { getLocaleSlug } from 'i18n-calypso';

		@@ -1,195 +1,4 @@
		import formatNumberCompact, { formatNumberMetric } from 'calypso/lib/format-number-compact';

		describe( 'formatNumberCompact', () => {

Reader: Fix Post Count Not Appearing #87864

Are you sure you want to change the base?

Reader: Fix Post Count Not Appearing #87864

Conversation

Aurorum commented Feb 25, 2024 • edited Loading

Proposed Changes

Testing Instructions

Choose a reason for hiding this comment

Aurorum commented Feb 25, 2024

Aurorum commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aurorum Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aurorum commented Feb 25, 2024 •

edited

Loading

Aurorum Mar 16, 2024 •

edited

Loading