Stats: use Redux for Post Trends #5847

timmyc · 2016-06-07T04:20:01Z

Part of moving stats away from the stats-list and localStore - this branch moves the Post Trends visualization on the insights page to use redux-backed data. In order to do so, a new data component QuerySiteStats has been added here, which will be utilized throughout the rest of the stats components to fetch data from the API.

Alongside the addition of QuerySiteStats, I realized the initial approach taken in #5743 WRT the response structure of the stats streak data was incorrect. The updates to the selectors and tests to fix this can be seen in 7c11823.

Integrating the new query component, and associated selector logic can be seen in b668a54.

To Test

Open up the stats insights page, validate the "Posting Activity" component up top looks/functions as it does in production
Switch sites to verify loading state and data updates between site changes

There is quite a bit more ES6-ification that can happen throughout the post-trends/*.jsx files, but I tried to keep this focused on the task at hand.

Test live: https://calypso.live/?branch=update/stats/insights-streak

timmyc · 2016-06-07T21:19:09Z

Ping @nylen if you are keen to dabble in stats land. Let me know if you want a walk through at all.

aduth · 2016-06-08T00:50:17Z

client/my-sites/stats/post-trends/day.jsx

 					className="chart__tooltip is-streak"
+					onClose={ noop }


Is the relocation an indication that you'd tried to remove the prop altoghether? 😉 That was my first thought, though looks like this is required in <Popover />. Don't see any reason why we couldn't make it optional... side-concern of course.

Indeed I did try to remove this, and of course eslint complains about the () => {} here. I agree we should make this an optional prop with a default of a noop in the base component. Are you okay with doing this in a follow-up?

timmyc · 2016-06-08T02:30:57Z

Thanks for the review @aduth. Applied most of the updates in d110b64 - including simplifying the popover and only persisting the data object from the API response in the state tree.

In e4f2b80 the old streakList logic was removed from the controller and upstream files.

timmyc · 2016-06-08T02:31:33Z

client/state/stats/lists/actions.js

@@ -53,7 +48,7 @@ export function requestSiteStats( siteId, statType, query ) {
 		} );

 		return wpcom.site( siteId )[ statType ]( query ).then( data => {
-			dispatch( receiveSiteStats( siteId, statType, query, omit( data, '_headers' ) ) );
+			dispatch( receiveSiteStats( siteId, statType, query, data.data ) );


Data Domination.

Tug · 2016-06-08T09:01:21Z

client/my-sites/stats/post-trends/day.jsx

-var i18n = require( 'lib/mixins/i18n' ),
-	Popover = require( 'components/popover' ),
-	Tooltip = require( 'components/chart/tooltip' );
+import i18n from 'lib/mixins/i18n';


Hi there, i18n usage has just changed recently. please use:

import i18n from 'i18n-calypso';

instead

JS sort() uses string comparisons by default, also looping over the object is quicker than doing a sort.

nylen · 2016-06-08T19:08:22Z

We've been iterating on this a bit, and I think it is looking good and working well.

One change we discussed is adding max: 3000 to the query objects in post-trends/index.jsx and post-trends/day.jsx. This only makes a difference for sites with very large numbers of posts but this way it will match the old behavior.

Since these query objects are duplicated, and must be the same between these two files for everything to work correctly, we should move them into a common function - perhaps state/stats/lists/utils.js is an appropriate location for that.

timmyc · 2016-06-08T19:10:58Z

Thanks for the sleuthing @nylen - I will add a util method and update the branch.

nylen · 2016-06-08T19:12:41Z

As far as I'm concerned, feel free to ship after that - I'm satisfied with how this works, unless @aduth wants to give the reduxy bits another pass.

timmyc · 2016-06-08T19:28:55Z

getStatsStreakQuery and tests added in a5af2ef

nylen · 2016-06-08T19:32:42Z

client/state/stats/lists/test/utils.js

+
+	describe( 'getStatsStreakQuery()', () => {
+		it( 'should return the correct default query', () => {
+			const streakQuery = getStatsStreakQuery( {} );


Could omit the {} here so that we test the default parameter value.

…calls.

timmyc · 2016-06-09T21:48:06Z

@nylen in 97e3526 I removed the statsStreak query logic, and made <Day /> a pure component. Only one connect now is happening in the parent component, and the streakData is passed down. Granted this removes the ability to have <Month /> and <Week /> be pure, it will result in fewer re-renders when a new payload arrives.

nylen · 2016-06-10T14:08:58Z

client/my-sites/stats/post-trends/week.jsx

+
+		for ( let i = 0; i < 7; i++ ) {
+			const dayDate = i18n.moment( startDate ).add( i, 'day' );
+			const postCount = streakData[ dayDate.format( 'YYYY-MM-DD' ) ];


Should probably add || 0 here since postCount is a number prop of Day.

I have a default prop value set to 0 for postCount on <Day />, but I suppose I could do that here too.

nylen · 2016-06-10T14:22:21Z

Still working well, a couple of minor notes but otherwise LGTM

timmyc · 2016-06-10T14:30:11Z

Changed up those items in 02a490a @nylen - good to merge this then?

timmyc added [Feature] Stats Everything related to our analytics product at /stats/ [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. State labels Jun 7, 2016

timmyc self-assigned this Jun 7, 2016

aduth reviewed Jun 8, 2016
View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 8, 2016

timmyc reviewed Jun 8, 2016
View reviewed changes

timmyc 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 Author Reply labels Jun 8, 2016

Tug reviewed Jun 8, 2016
View reviewed changes

timmyc added 6 commits June 8, 2016 09:04

Fix streak selector logic to match API response and add Query component.

cfa3970

Use redux in PostTrends.

843d057

Updates per feedback

6d9c8df

Remove old streakList logic

62cef08

Change i18n import.

c3d0852

Fix linting issues.

f3c5c6f

timmyc force-pushed the update/stats/insights-streak branch from e4f2b80 to f3c5c6f Compare June 8, 2016 16:40

nylen added 3 commits June 8, 2016 13:31

Fix getSiteStatsMaxPostsByDay logic

9f2837b

JS sort() uses string comparisons by default, also looping over the object is quicker than doing a sort.

Remove extraneous 'taxonomy' parameter

4d7b773

Add previously failing test for getSiteStatsMaxPostsByDay

32766f5

timmyc added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 8, 2016

Add utility function for default statsStreak query

a5af2ef

timmyc added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jun 8, 2016

nylen reviewed Jun 8, 2016
View reviewed changes

timmyc added 2 commits June 8, 2016 20:43

Minor change to test case.

f3b8862

Make Day a pure component for least amount of re-renders and connect …

97e3526

…calls.

nylen reviewed Jun 10, 2016
View reviewed changes

Updates per feedback.

02a490a

nylen added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 10, 2016

nylen mentioned this pull request Jun 10, 2016

Popover: Make onClose optional #5959

Closed

timmyc merged commit cf69f56 into master Jun 10, 2016

matticbot removed the [Status] Ready to Merge label Jun 10, 2016

timmyc deleted the update/stats/insights-streak branch June 10, 2016 16:51

timmyc mentioned this pull request Jun 10, 2016

Stats: use Redux for statsSummary #5975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats: use Redux for Post Trends #5847

Stats: use Redux for Post Trends #5847

timmyc commented Jun 7, 2016 •

edited by matticbot

Loading

timmyc commented Jun 7, 2016

aduth Jun 8, 2016

timmyc Jun 8, 2016

nylen Jun 10, 2016

timmyc commented Jun 8, 2016

timmyc Jun 8, 2016

Tug Jun 8, 2016

nylen commented Jun 8, 2016

timmyc commented Jun 8, 2016

nylen commented Jun 8, 2016

timmyc commented Jun 8, 2016

nylen Jun 8, 2016

timmyc commented Jun 9, 2016

nylen Jun 10, 2016

timmyc Jun 10, 2016

nylen commented Jun 10, 2016

timmyc commented Jun 10, 2016

Stats: use Redux for Post Trends #5847

Stats: use Redux for Post Trends #5847

Conversation

timmyc commented Jun 7, 2016 • edited by matticbot Loading

timmyc commented Jun 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmyc commented Jun 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Jun 8, 2016

timmyc commented Jun 8, 2016

nylen commented Jun 8, 2016

timmyc commented Jun 8, 2016

Choose a reason for hiding this comment

timmyc commented Jun 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nylen commented Jun 10, 2016

timmyc commented Jun 10, 2016

timmyc commented Jun 7, 2016 •

edited by matticbot

Loading