Post Anniversaries #14991

bpierre · 2017-06-11T18:55:35Z

Hi!

This is my first pull request on the project, its goal is to add a module in the stats section that will display the post anniversaries corresponding to the current day. Please let me know what you think! 😊

Screenshots

Here are some visuals of the module in its current state, including zero / one / two / more posts:

Rationale

The first approach I followed was to provide a full completeness: the module displays all the posts found for the last ten years [1].

Another approach that I would like to discuss would be to see this module as something more “entertaining” than informative, which would allow it to display only one (random) anniversary if several exist. It could also visually be identified as something special (e.g. using an icon representing a birthday cake), the same way “Best Views Ever” is using a cup icon with a yellow color.

Display modes

I believe (maybe this information can be found somewhere?) that having only one or zero anniversaries would be the most common cases, and decided to display the single anniversary case as a simple sentence, following the pattern of the “Latest Post Summary” module:

When several anniversaries exist, a table is used to display the list of posts, reusing a component found in other stats modules like “Posts & Pages”:

While having the advantage of reusing an existing component, I am not sure a table is the best way to display this data. Another way could be to only display the first year containing anniversaries, and to add buttons that would allow the user to navigate through the years containing anniversaries. The posts could also be grouped by year as a series of lists. If so, a solution could be to expand it vertically using a “more” button.

Emplacement

At the moment, the module appears in the Insights tab of the stats section. I am still unsure about this, as my first instinct was to put it in the Days tab.

Some reasons for not displaying it in the Days tab:

It would be the only module in the Days tab that wouldn’t be displayed in the Weeks, Months, and Years tabs.
It would be the only module in the Days tab not referring to the visits.
The modules displayed in the Insights tab are already about different aspects.

But an important reason for doing it would be to allow the user to display the post anniversaries of any day, not only today.

Additional Notes

I decided to not put it behind a feature flag as the project is small enough, but maybe I should do it, so the PR can be merged early?
After thinking about the wording, I decided to use “Anniversaries” instead of “Post Anniversaries” in the module, as the context should be enough to understand that we are talking about posts. Please tell me if you think that it should be changed.
I used calypso-prettier (Framework: Use Prettier? #12260) to format the code of the module. I had very good results with it, but it might be a bit premature. Please tell me if this is the case, and I will reformat it manually.
I modified the module stats/stats-list/stats-list-item.jsx to allow it to display a year without formatting it as a number.
I used a wrapper around the <Gridicon/> component to fix its alignment. It is a temporary fix that I only applied to the single anniversary mode. The problem is present with other modules of the stats section and I will open a separate issue to discuss this, so this fix can be removed.

cc @ehg

[1] This limitation exists because the WordPress.com API doesn’t allow a query on multiple date ranges, which means that each year need to be queried separately. The module tries to optimize this by fetching the oldest post first. Then, if the blog is younger than ten years, only the relevant years will be queried. This optimization can easily be removed in the future whenever / if the WordPress.com API implements the feature.

a8ci18n · 2017-06-11T19:00:36Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+
+const EmptyMessage = localize( ( { translate } ) => (
+	<ErrorPanel
+		message={ translate( 'No anniversaries today!', {


ℹ️ This context is really long. Are you sure you don't want to use a translator comment instead?

a8ci18n · 2017-06-11T19:00:36Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+	<div>
+		<StatsContentText>
+			<p>
+				{ translate( 'Published on this day in the past years:', {


ℹ️ This string is probably long enough that it doesn't need a context. Are you sure you don't want to use a translator comment instead?

a8ci18n · 2017-06-11T19:00:36Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+	return (
+		<div>
+			<SectionHeader
+				label={ translate( 'Anniversaries', {


Hi! I've found a possible matching string that has already been translated 27 times:
translate( 'Happy Anniversary!' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

jonathansadowski · 2017-06-14T16:21:54Z

Awesome stuff -- thanks for taking the time to think this through @bpierre -- as well as for so clearly laying out the thought process behind the decisions you've made.

Another approach that I would like to discuss would be to see this module as something more "entertaining" than informative, which would allow it to display only one (random) anniversary if several exist. It could also visually be identified as something special (e.g. using an icon representing a birthday cake), the same way "Best Views Ever" is using a cup icon with a yellow color.

That's an interesting approach. It might also be cool to see the post that is the most notable anniversary (based on views or some other metric).

I believe (maybe this information can be found somewhere?) that having only one or zero anniversaries would be the most common cases

I believe you are correct. In fact, for the first year of a user's life, this module wouldn't show any aniversaries. It might be interesting to see the module only appear when anniversaries exist, as opposed to having a "zero state".

At the moment, the module appears in the Insights tab of the stats section.

I agree with your reasoning for placing the module on Insights.

But an important reason for doing it would be to allow the user to display the post anniversaries of any day, not only today.

Ideally we'd have a way for you to see stats for any day (currently you can only navigate as far back as the calendar allows), which would make anniversaries on a specific day a little less notable.

folletto · 2017-06-15T14:58:47Z

Another approach that I would like to discuss would be to see this module as something more “entertaining” than informative, which would allow it to display only one (random) anniversary if several exist.

I agree would be really nice. The approach I'd take is to do this first, then to iterate with the more entertaining one.

You raise also some good questions and considerations, I'll try to summarize it:

I would display it in Days, Weeks, Months. Doesn't show for Years.
When there's no data, the box shouldn't appear at all.
Placement should be for now at the bottom, as a consequence of not showing up all the time. I understand it's not ideal, but I take this as a starting point.
No need of feature flag, it's small enough.

In general, I'd also suggest to not "pack" multiple extra things in a PR. Try to keep the PRs focused on a single thing. ;)

folletto

Some changes

folletto · 2017-06-15T14:59:59Z

client/my-sites/stats/stats-list/stats-list-item.jsx

@@ -210,6 +210,9 @@ module.exports = React.createClass( {
 		}

 		switch ( valueData.type ) {
+			case 'raw':
+				value = valueData.value;
+				break;


Can you detail why this?

This is required to disable the formatting on the right column of the table. Without this, the value would be formatted as a number, e.g. 2,017 instead of 2017.

folletto · 2017-06-15T15:00:39Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+};
+
+const CalendarIcon = () => (
+	<span style={ { position: 'relative', top: '-2px' } }>


Please don't do this, and don't inline style. Style should be in its separate SCSS file.

If it's a more general issue, just open a separate issue.

Understood.

bpierre · 2017-06-15T19:12:21Z

Thank you for your feedback and suggestions!

@jonathansadowski

It might be interesting to see the module only appear when anniversaries exist, as opposed to having a "zero state".

Good idea! I am adding this.

@folletto

I would display it in Days, Weeks, Months. Doesn't show for Years.

Are you suggesting to remove it from Insights, or to also add it in these tabs?

How do you see it for the Weeks and Months tabs? A few options I have in mind:

Display the post anniversaries of the current day, no matter what week / month is selected (the easiest, but it might not be what the user expect).
Display the post anniversaries for the first day of the selected week / month.
Display the post anniversaries of the current day if the current week / month is selected, otherwise the first day of the selected week / month.
Display the post anniversaries for the entire week / month. This could represent an important amount of posts, and we should probably limit it or allow the user to navigate through the posts.

Placement should be for now at the bottom, as a consequence of not showing up all the time.

I agree, I’ll modify this.

In general, I'd also suggest to not "pack" multiple extra things in a PR. Try to keep the PRs focused on a single thing. ;)

I will try to do that, thanks.

a8ci18n · 2017-06-15T20:45:05Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+				: <div>
+						<SectionHeader
+							label={ translate(
+								'Anniversaries',


Hi! I've found a possible matching string that has already been translated 27 times:
translate( 'Happy Anniversary!' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

folletto · 2017-06-19T17:27:32Z

Are you suggesting to remove it from Insights, or to also add it in these tabs?

Remove from insights. Insights should be just "overviews", not tied to a definite timeframe.

Display the post anniversaries for the entire week / month. This could represent an important amount of posts, and we should probably limit it or allow the user to navigate through the posts.

Should show "all in the timeframe", so no to any arbitrary selection of day (the other options listed). This last option is the one we should frame, and I agree might be many posts. A simple way to approach this is to just show "top 3" and then a counter or link that expands on a separate page.

The MVP of this is probably to just rename the title to "Top Three Anniversaries" if there are more than X, and show top X in the box, where "Top" is by view count (we should have already the data in the API judging from https://wordpress.com/stats/year/posts/$site?startDate=2017-01-01. Let's keep this simple, if this is too complex API wise, let's devise a different way that allows us to have the box in the right place still :)

a8ci18n · 2017-06-26T17:18:38Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+		const date = period.startOf.clone();
+
+		if ( period.period === 'week' ) {
+			return translate( '%(startDate)s - %(endDate)s', {


Hi! I've found a possible matching string that has already been translated 32 times:
translate( '%(startDate)s - %(endDate)s', { context: 'Date range for which stats are being displayed'} ) ES Score: 27

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

ℹ️ This string is probably long enough that it doesn't need a context. Are you sure you don't want to use a translator comment instead?

a8ci18n · 2017-06-26T17:18:38Z

client/my-sites/stats/summary/index.jsx

@@ -186,6 +187,15 @@ class StatsSummary extends Component {
 					statType="statsSearchTerms"
 					summary />;
 				break;
+
+			case 'anniversaries':
+				title = translate( 'Anniversaries' );


Hi! I've found a possible matching string that has already been translated 27 times:
translate( 'Happy Anniversary!' ) ES Score: 11

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-26T17:18:39Z

client/my-sites/stats/stats-post-anniversaries/single-post.jsx

+		return translate( 'this week' );
+	}
+	if ( period.period === 'month' ) {
+		return translate( 'this month' );


Hi! I've found a possible matching string that has already been translated 41 times:
translate( 'This Month' ) ES Score: 12
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

a8ci18n · 2017-06-26T17:18:39Z

client/my-sites/stats/stats-post-anniversaries/single-post.jsx

+		return translate( 'this day' );
+	}
+	if ( period.period === 'week' ) {
+		return translate( 'this week' );


Hi! I've found a possible matching string that has already been translated 69 times:
translate( 'This week' ) ES Score: 13
See 1 additional suggestion in the PR translation status page

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

Changes following the suggestions on #14991: - Dedicated module page. - Move the module from the Insights tab to Days, Weeks and Months. - Shows all the anniversaries in the selected range. - Only display the top (by views) three posts anniversaries if more exist. The top three posts are selected by views count, but they are sorted by date and not by views count. - Open the post stat page when the link is clicked (consistency with other modules). - Move the module internal components in separate files.

bpierre · 2017-06-26T19:57:37Z

@folletto @jonathansadowski I pushed some changes, here is a description of the main things:

The module has been moved from the Insights tabs to the Days, Weeks and Months tabs. The anniversaries displayed in the module correspond to the selected date range.

The module now provides a dedicated page, that allows the user to see all the anniversary posts for a given Day, Week or Month. The page can be accessed using the module title:

When there are more than three posts in the module, only the three most viewed posts are displayed, the title becomes “Top 3 anniversaries”, and a “View All” footer link appears, which links to the page:

Dedicated page (for a month here):

Some notes

Requesting the posts views is made in a different way than the “Posts & Pages” modules, as we already have a precise list of posts for which the views need to be fetched. The solution is to make one <QueryPostStats /> request per post, which shouldn’t be much of a problem with HTTP2, but it could be nice to have the possibility of querying the stats of several posts at once on <QueryPostStats />. Note that these requests only happen if the total exceeds the maximum amount of posts (3).

The posts now link to the stats page of the post, for consistency with the other modules. The post itself is still accessible with an “View” / external icon link (matching the other modules too).

Some components are imported from the <StatsModule /> internal components. The Posts Anniversary module is too different to be integrated into it, but maybe we could think about how to separate its pure rendering components from its queries, to make them easier to reuse with other components.

folletto · 2017-06-27T09:53:19Z

Design wise, looks good!

I'd do just one final tweak:

We might also drop the icon, as takes away some title, what do you think?

bpierre · 2017-06-27T10:08:31Z

I agree, let’s remove this.

Do you think it should be removed from the single anniversary mode as well? I think it would be nice to have some sort of a visual emphasis, but I am not convinced by having the calendar icon here.

folletto · 2017-06-27T10:31:45Z

I agree with you it's kinda nice when it's just one, without the repetition. It also doesn't limit the title length in that format, so it's good. 👍

mattsherman

Gave a look through the code and quickly tested the branch (not exhaustively). Left a few comments for things that I'd like to see addressed.

mattsherman · 2017-07-06T18:43:26Z

client/my-sites/stats/stats-post-anniversaries/grouped-posts.jsx

+
+const label = ( translate, period ) => {
+	if ( period.period === 'day' ) {
+		return translate( 'Published on this day in the past years:', {


Change to "Published on this day in the past:"

mattsherman · 2017-07-06T18:43:51Z

client/my-sites/stats/stats-post-anniversaries/grouped-posts.jsx

+		} );
+	}
+	if ( period.period === 'week' ) {
+		return translate( 'Published on this week in the past years:', {


Change to "Published on this week in the past:"

mattsherman · 2017-07-06T18:44:28Z

client/my-sites/stats/stats-post-anniversaries/grouped-posts.jsx

+		} );
+	}
+	if ( period.period === 'month' ) {
+		return translate( 'Published on this month in the past years:', {


Change to "Published on this month in the past:"

mattsherman · 2017-07-06T18:46:53Z

client/my-sites/stats/stats-post-anniversaries/grouped-posts.jsx

+		<StatsListLegend label={ translate( 'Title' ) } value={ translate( 'Year' ) } />
+		<StatsList
+			moduleName={ 'postAnniversaries' }
+			data={ postsByYear.reduce(


Move this generation of the data to a separate function -- will make the JSX clearer to read.

mattsherman · 2017-07-06T19:13:26Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+
+	const oldestPostQuery = { number: 1, order: 'ASC' };
+
+	const mapStateToProps = ( state, props ) => {


This method is too long, please refactor to split up.

mattsherman · 2017-07-06T19:30:53Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+			if ( postsByYear[ i ] !== result ) {
+				postsByYear[ i ] = result || [];
+				postsByYear = [ ...postsByYear ];
+			}


I find this code section confusing. Please consider reworking to make it a bit more understandable at a glance.

mattsherman · 2017-07-06T19:34:56Z

client/my-sites/stats/stats-post-anniversaries/single-post.jsx

+				<Gridicon icon="calendar" size={ 18 } />
+				{ translate(
+					'%(yearsAgo)d year ago on %(period)s, {{href}}%(title)s{{/href}} was published.',
+					'%(yearsAgo)d years ago on %(period)s, {{href}}%(title)s{{/href}} was published.',


Concatenating translated strings (%period) like this can result in issues for some languages. Please rework to avoid inserting translated strings instead other translated strings.

ehg · 2017-07-07T10:17:39Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+					? null
+					: <div>
+							{ topPostsOnly
+								? allPosts.map(


This worries me. Sure, HTTP2 requests are slimmer for the client, but won't the server still have to process these requests? How is this bounded?

Indeed, thanks for noticing this. The maximum amount of requests is set 10 to fetch the posts, but to create the top 3, a single request need to be created for each individual post, and even a single day can have an infinite amount of posts. I just opened a pull request that will add the possibility to fetch the top posts for a period of time, which will allow to make a single request per period of time in order to get the most viewed posts.

ehg · 2017-07-07T10:27:53Z

client/my-sites/stats/stats-post-anniversaries/index.jsx

+	status: 'publish',
+} );
+
+export default connect( () => {


Having lots of logic in here indicates a code smell to me, and makes me think certain logic belongs in selectors?

Stats: add a module to display the post anniversaries for the current day. The stats-list-item module has also been modified to display a value without formatting.

Changes following the suggestions on #14991: - Dedicated module page. - Move the module from the Insights tab to Days, Weeks and Months. - Shows all the anniversaries in the selected range. - Only display the top (by views) three posts anniversaries if more exist. The top three posts are selected by views count, but they are sorted by date and not by views count. - Open the post stat page when the link is clicked (consistency with other modules). - Move the module internal components in separate files.

- Removed nested translations - Made some parts easier to understand

bpierre · 2017-07-14T13:11:22Z

@mattsherman Thanks for your feedback! I made the requested changes, apart from the connect() refactoring that I plan to do while working on a new solution using QueryTopPosts.

mattsherman · 2017-07-18T19:29:12Z

@bpierre Thanks for making those changes. I will also take a look at your related PR #16232 .

bpierre added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jun 11, 2017

matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels Jun 11, 2017

a8ci18n reviewed Jun 11, 2017

View reviewed changes

ehg assigned folletto and mtias and unassigned folletto and mtias Jun 12, 2017

ehg requested review from folletto and mtias June 12, 2017 14:51

ehg assigned bpierre Jun 12, 2017

ehg added the [Status] In Progress label Jun 12, 2017

bpierre force-pushed the add/stats/anniversary-posts branch from be022e6 to 6f176a9 Compare June 13, 2017 23:07

folletto suggested changes Jun 15, 2017

View reviewed changes

bpierre force-pushed the add/stats/anniversary-posts branch from 6f176a9 to 4996efb Compare June 15, 2017 20:39

a8ci18n reviewed Jun 15, 2017

View reviewed changes

bpierre added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jun 21, 2017

bpierre force-pushed the add/stats/anniversary-posts branch from 7c8fb09 to e896d74 Compare June 26, 2017 17:12

a8ci18n reviewed Jun 26, 2017

View reviewed changes

bpierre force-pushed the add/stats/anniversary-posts branch from e896d74 to 9123cf7 Compare June 26, 2017 18:20

bpierre force-pushed the add/stats/anniversary-posts branch from 9123cf7 to 0594adb Compare June 26, 2017 18:25

bpierre force-pushed the add/stats/anniversary-posts branch from 0594adb to bf2e59e Compare June 26, 2017 18:44

bpierre force-pushed the add/stats/anniversary-posts branch from bf2e59e to 01cfb82 Compare June 26, 2017 19:35

bpierre removed the [Status] In Progress label Jul 4, 2017

mattsherman suggested changes Jul 6, 2017

View reviewed changes

ehg reviewed Jul 7, 2017

View reviewed changes

bpierre mentioned this pull request Jul 14, 2017

Add a top-posts component and state module #16232

Closed

bpierre added 7 commits July 14, 2017 13:58

Post Anniversaries

b063404

Stats: add a module to display the post anniversaries for the current day. The stats-list-item module has also been modified to display a value without formatting.

Post Anniversaries: don’t display the box for blogs younger than a year

f2cd600

Post Anniversaries: only display the box when there are posts

1874dcb

Post Anniversaries: remove the icon alignment fix

a708b7e

Stats Anniversaries: remove icons in the list view

864299e

Stats Anniversaries: code structure

5140391

- Removed nested translations - Made some parts easier to understand

bpierre force-pushed the add/stats/anniversary-posts branch from b8b8b11 to 5140391 Compare July 14, 2017 12:58

bpierre closed this Sep 23, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 23, 2017

alisterscott deleted the add/stats/anniversary-posts branch October 24, 2017 04:43


		const oldestPostQuery = { number: 1, order: 'ASC' };

		const mapStateToProps = ( state, props ) => {

Post Anniversaries #14991

Post Anniversaries #14991

Conversation

bpierre commented Jun 11, 2017

Screenshots

Rationale

Display modes

Emplacement

Additional Notes

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

a8ci18n Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

jonathansadowski commented Jun 14, 2017

folletto commented Jun 15, 2017

folletto left a 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

bpierre commented Jun 15, 2017

Choose a reason for hiding this comment

folletto commented Jun 19, 2017 • 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

bpierre commented Jun 26, 2017

Some notes

folletto commented Jun 27, 2017

bpierre commented Jun 27, 2017

folletto commented Jun 27, 2017

mattsherman left a 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

Choose a reason for hiding this comment

bpierre commented Jul 14, 2017

mattsherman commented Jul 18, 2017

a8ci18n Jun 11, 2017 •

edited

Loading

folletto commented Jun 19, 2017 •

edited

Loading