Stats: create redux version of stats-list. #6135

timmyc · 2016-06-18T20:02:26Z

As part of migrating from the legacy stats-list to utilizing the global state tree across stats, this branch introduces a new version of the <StatsModule /> component. The <StatsConnectedModule /> will help ease the transition of all legacy <StatsModule /> usage to utilize Redux.

I attempted to cobble together support for redux + stats-list within the existing <StatsModule /> but after fighting backwards compatibility - I felt creating a new temporary module to use during the transition was the best approach.

The first stats module to use this new Connected component is the "Publicize" module on the Insights page. This is a very basic module with no row actions or summary page support - so it seemed like a great place to start.

To Test

Visit a site insights page
Validate the Publicize module loads as expected, and shows the same data as seen in production

Implementation Notes
Additionally this branch introduces a new selector getSiteStatsParsedData. Akin to the legacy stats-list, this selector will call the appropriate utility method in state/stats/lists/utils for the given statType. In this branch you can see tests added for this new selector along with tests for parsing statsPublicize data.

After all existing stats modules are updated to use <StatsConnectedModule /> my plan will be to delete the old version of <StatsModule /> and let the connected version become the default use case. This approach will allow us to do smaller/focused PRs to migrate all stats modules over to redux.

Test live: https://calypso.live/?branch=update/stats/publicize

aduth · 2016-06-28T00:39:39Z

client/my-sites/stats/stats-module/connected-list.js

+
+	return {
+		requesting: isRequestingSiteStatsForQuery( state, siteId, statType, query ),
+		siteSlug: site ? site.slug : '',


slug shouldn't exist as a property on site, and if it does, we have bigger problems 😄

slug is a computed property added by the legacy lib/site module, you should use the getSiteSlug selector instead.

But I'm very curious if site.slug is returning a value here.

This particular component never renders a summary link, but to answer your question, yes slug is on the site object from the state tree here:

But this seems valid to me, as slug is defined on the schema

Well, thank you for finding this at least. Looks like folks have been adding back computed properties originally removed in #4060. See discussion at #5966 (comment).

timmyc · 2016-06-28T23:48:15Z

Thanks for the review @aduth. I added the Parsers concept to utils in #6013 - I'm going to mark this one was in-progress until that PR lands then rebase and take care of the other items here then.

I will do my best to clean-up the classNames items as I go here - there is some interesting stuff in stats .scss land still, so I don't want to scope creep too far from the redux goals in these PRs.

timmyc · 2016-06-29T02:32:51Z

Cleaned up some of the className issues - some were not even being used for anything 😆 - but extracted one source to a new component <StatsModuleExpand /> which can be utilized in other components too.

timmyc · 2016-06-29T20:49:50Z

Rebased and ready for another review.

aduth · 2016-06-30T12:38:09Z

client/my-sites/stats/stats-module/connected-list.js

+			return '/stats/' + period.period + '/' + path + '/' + siteSlug + '?startDate=' + date;
+		}
+
+		return null;


Minor and maybe arguable to have the explicitness, but you could omit the fallback return, as the default return value undefined would work just as well for your use case.

aduth · 2016-06-30T12:41:23Z

client/state/stats/lists/utils.js

+	 * @param  {Object} data Stats query
+	 * @return {Array}       Parsed publicize data array
+	 */
+	statsPublicize: ( data = {} ) => {


Minor again: This can be shortened to:

statsPublicize( data = {} ) {

aduth · 2016-06-30T12:43:49Z

Perhaps more suitable for a future pull request (though refactoring here reimplements the logo-ing anyways), but we should drop the logo images in favor of <SocialLogo /> (demo, docs).

aduth · 2016-06-30T12:48:46Z

client/my-sites/stats/stats-module/expand.jsx

+import React from 'react';
+import PureComponent from 'react-pure-render/component';
+
+export default class StatsModuleExpand extends PureComponent {


I was going to suggest this would make a good stateless function component, though one downside being it's not possible to have pure stateless function components without using something like connect or pulling in recompose.

aduth · 2016-06-30T12:56:36Z

I see two network requests for GET /sites/%s/stats/publicize when viewing the insights page. Possibly another module also issuing a request from legacy logic?

aduth · 2016-06-30T13:02:00Z

This is working well in my testing. A few questions noted, but otherwise good to merge I think 👍

timmyc · 2016-06-30T19:10:43Z

I see two network requests for GET /sites/%s/stats/publicize when viewing the insights page. Possibly another module also issuing a request from legacy logic?

Good catch, the removal of the old list logic in the controller was lost during my rebase. Fixed in 86fb690

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 18, 2016

timmyc self-assigned this Jun 18, 2016

aduth reviewed Jun 28, 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 28, 2016

timmyc mentioned this pull request Jun 28, 2016

Stats: update all-time component to use QuerySiteStats. #6013

Merged

timmyc added [Status] In Progress and removed [Status] Needs Author Reply labels Jun 28, 2016

timmyc force-pushed the update/stats/publicize branch from 57ff9ce to 286ebb4 Compare June 29, 2016 02:30

timmyc force-pushed the update/stats/publicize branch 2 times, most recently from cae0b81 to 67bc660 Compare June 29, 2016 20:40

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 29, 2016

aduth reviewed Jun 30, 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 30, 2016

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 30, 2016

timmyc added 6 commits July 2, 2016 12:10

Stats: create redux version of stats-list.

1cc0aab

Additional refactoring of connected-list, add expand component.

ed70748

Refactor statsPublicize normalizer.

67ba54d

Remove un-needed div.

9e5077d

Updates per feedback.

628a2a0

Clean up publicizeList in stats controller.

7e6429a

timmyc force-pushed the update/stats/publicize branch from 86fb690 to 7e6429a Compare July 2, 2016 21:51

timmyc merged commit 51755f6 into master Jul 2, 2016

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

timmyc deleted the update/stats/publicize branch July 2, 2016 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats: create redux version of stats-list. #6135

Stats: create redux version of stats-list. #6135

timmyc commented Jun 18, 2016 •

edited by matticbot

Loading

aduth Jun 28, 2016

timmyc Jun 29, 2016

aduth Jun 30, 2016

timmyc commented Jun 28, 2016 •

edited

Loading

timmyc commented Jun 29, 2016

timmyc commented Jun 29, 2016

aduth Jun 30, 2016 •

edited

Loading

aduth Jun 30, 2016

aduth commented Jun 30, 2016

aduth Jun 30, 2016

aduth commented Jun 30, 2016

aduth commented Jun 30, 2016

timmyc commented Jun 30, 2016

Stats: create redux version of stats-list. #6135

Stats: create redux version of stats-list. #6135

Conversation

timmyc commented Jun 18, 2016 • edited by matticbot Loading

aduth Jun 28, 2016

Choose a reason for hiding this comment

timmyc Jun 29, 2016

Choose a reason for hiding this comment

aduth Jun 30, 2016

Choose a reason for hiding this comment

timmyc commented Jun 28, 2016 • edited Loading

timmyc commented Jun 29, 2016

timmyc commented Jun 29, 2016

aduth Jun 30, 2016 • edited Loading

Choose a reason for hiding this comment

aduth Jun 30, 2016

Choose a reason for hiding this comment

aduth commented Jun 30, 2016

aduth Jun 30, 2016

Choose a reason for hiding this comment

aduth commented Jun 30, 2016

aduth commented Jun 30, 2016

timmyc commented Jun 30, 2016

timmyc commented Jun 18, 2016 •

edited by matticbot

Loading

timmyc commented Jun 28, 2016 •

edited

Loading

aduth Jun 30, 2016 •

edited

Loading