Exploration: Top-down removal of SitesList, starting from Layout #13094

mcsf · 2017-04-13T16:56:12Z

Do not merge Yay!

This PR seeks to identity pinpointable pain points for project Remove SitesList by starting from Layout, requesting all sites via QuerySites, and going down from there. Points of interest include my-sites/controllers, navigation, site switching.

matticbot · 2017-04-13T16:56:13Z

Test live: https://calypso.live/?branch=try/layout-query-sites

gwwar · 2017-04-14T16:42:25Z

client/my-sites/controller.js

 		const siteID = route.getSiteFragment( context.path );
 		const basePath = route.sectionify( context.path );
 		const currentUser = user.get();
 		const hasOneSite = currentUser.visible_site_count === 1;
 		const allSitesPath = route.sectionify( context.path );
+		const primaryId = getPrimarySiteId( state );
+		const primary = getSite( state, primaryId ) || '';
+		const hasInitialized = !! getSites( state );


Is the intention here to wait for fresh data, or is persisted data fine? This will be true if we have some sites in indexedDB.

Indeed — this has to be solved some other way.

ockham · 2017-04-14T19:34:18Z

client/components/site-selector/index.jsx

+		const site = this.props.getSiteBySlug( slug );
+		console.warn( 'getSiteBySlug', site, slug );
+
+		// @FIXME move to selector


WIP at #13123

mcsf · 2017-04-15T14:52:35Z

Refactored on top of some selector additions. my-sites/controller is the current focus of this. So far, in that module, everything seems to have been replaced with Redux selections and action dispatches, except:

A call to isATEnabledForCurrentSite, a part of lib/automated-transfer, which will have to be de-SitesList'd separately → Framework: Remove SitesList usage from lib/automated-transfer #13139
Calls to sites.once
Reads from and writes to sites.initialized

mtias · 2017-04-17T18:00:06Z

client/boot/project/wordpress-com.js

-	}
+	// @FIXME don't use sitesFactory
+	//if ( config.isEnabled( 'keyboard-shortcuts' ) ) {
+	//	require( 'lib/keyboard-shortcuts/global' )( sitesFactory() );


fwiw, I'd be ok with disabling this if it becomes a blocker.

Being fixed in #13206

It's merged 👍

mtias · 2017-04-17T18:01:03Z

client/components/site-selector/index.jsx

@@ -118,7 +133,8 @@ const SiteSelector = React.createClass( {
 			highlightedIndex = this.state.highlightedIndex;
 		} else if ( this.lastMouseHover ) {
 			debug( `restoring highlight from last mouse hover (${ this.lastMouseHover })` );
-			highlightedSite = this.props.sites.getSite( this.lastMouseHover ) || this.lastMouseHover;
+			// @FIXME
+			highlightedSite = this.props.getSiteBySlug( this.lastMouseHover ) || this.lastMouseHover;


This is all really odd and hard to read, but let's push any bigger refactoring (lastMouseHover being a slug) for after sites-list is done.

mtias · 2017-04-17T18:02:38Z

client/state/sites/selectors.js

+		const site = getRawSite( state, siteId ) ||
+			// Support for non-ID site retrieval
+			// Replaces SitesList#getSite
+			getSiteBySlug( state, siteId );


Should this be done in getRawSite instead?

There would be a circular dependency between getRawSite and getSiteBySlug via getSiteSlug.

ockham · 2017-04-18T19:05:54Z

client/layout/index.jsx

@@ -143,6 +143,7 @@ Layout = React.createClass( {
 		return (
 			<div className={ sectionClass }>
 				<DocumentHead />
+				<QuerySites allSites />


Instead of Layout, should this maybe live in my-sites/sidebar (and use selectedSiteId in single-site mode there)? That'd seem a bit more fine-grained to me. Not all sections need (all) sites data...

I think the motivation was to be more preemptive in fetching sites data, despite not all sections requiring it. For now, I'd be inclined to leave it in Layout, but I don't feel strongly about either.

We use sites data for sites popover in every section and in the masterbar, that was the main reason. Also, because it replaces fetching we were doing at controller level.

If we need to exclude from a certain section, we can do the checks needed in Layout. But I'm fine with this. Sites is pretty instrumental for the whole calypso experience.

Yes, it makes sense. Sooner or later all logged in sections need sites list ...

If we're adding this to layout can we clean up other usages of <QuerySites allSites /> in

client/my-sites/plugins/plugin.jsx

client/my-sites/stats/overview.jsx

client/signup/jetpack-connect/index.jsx

client/signup/jetpack-connect/authorize-form.jsx

@mcsf could we clean up these? ^^

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

* Import was kept, as `sites` is passed as props in one controller. To be fixed in a separate PR.

- :-( - Aims to fix regressions found, notably around flows in `/domains`

This mimics the behavior that used to be in place with `sites.initialized`. Without it, render jumps may happen as we default to a site-less view then correct to a site-specific one.

Component logic expects non-number `count`, e.g. when no site is selected. This eliminates React type warnings.

One of the async jobs of fetchSitesAndUser is to trigger a `fetch` on `lib/sites-list`, wait for changes to that structure, check for the local availability of data for the new site, then either continue or retrigger a fetch. This commit ports that behavior to our Redux data layer.

Before #13094, this behavior was ensured by the `acceptInvite` action creator, which dispatches old Flux actions that `lib/sites-list` listens to in order to call a `sync` on itself.

blowery · 2017-05-09T14:34:37Z

client/components/site-selector/index.jsx

 		showRecentSites: get( user, 'visible_site_count', 0 ) > 11,
 		recentSites: getPreference( state, 'recentSites' ),
 		siteCount: get( user, 'site_count', 0 ),
 		visibleSiteCount: visibleSiteCount,
+		getSite: getSite.bind( null, state ),


isn't this going to make the component effectively impure? every time connect runs, this will be a different function and shallowEquals will fail...

ah, you covered this later on in https://github.com/Automattic/wp-calypso/pull/13094/files#r112195983

@blowery: correct, and it's been a priority of mine to tackle these. There were other such occurrences that we got rid of in the meantime, but this one stayed. Now that this is merged, it unblocks some other work around SiteSelector, Site, etc., which seeks to make more parts only care about site IDs and not need to satisfy as much data. /cc @ockham

…at all Fixes a bug where `SiteSelector` would show a placeholder instead of list of visible sites, even when the sites list data are available. The `isRequestingMissingSites` selector returns `true` when there is an outstanding request for the sites data and the local data are detected as not-fresh, i.e., the local site count is different from the server site count, as revealed by the `user.site_count` property. In such a case, a placeholder is shown instead of showing the available local list. This is a regression introduced in #13094, when `sites-list` was removed from the component. The condition to show the placeholder used to be `! sites.initialized`, which is much weaker than `isRequestingMissingSites`. The `sites-list` could be initialized from local storage data and be requesting an update from the server, without affecting the `initialized` flag. This patch replaces the `isRequestingMissingSites` selector with `hasLoadedSites`, which is more appropriate here. It also fixes the `disabled` condition for the `Search` component. It used to be `! this.props.sites`, but `this.props.sites` is always truthy, because the `getSites` selector returns an empty object when there are no site data. This could happen when the sites list data were detected as not-fresh

…at all Fixes a bug where `SiteSelector` would show a placeholder instead of list of visible sites, even when the sites list data are available. The `isRequestingMissingSites` selector returns `true` when there is an outstanding request for the sites data and the local data are detected as not-fresh, i.e., the local site count is different from the server site count, as revealed by the `user.site_count` property. In such a case, a placeholder is shown instead of showing the available local list. This is a regression introduced in Automattic#13094, when `sites-list` was removed from the component. The condition to show the placeholder used to be `! sites.initialized`, which is much weaker than `isRequestingMissingSites`. The `sites-list` could be initialized from local storage data and be requesting an update from the server, without affecting the `initialized` flag. This patch replaces the `isRequestingMissingSites` selector with `hasLoadedSites`, which is more appropriate here. It also fixes the `disabled` condition for the `Search` component. It used to be `! this.props.sites`, but `this.props.sites` is always truthy, because the `getSites` selector returns an empty object when there are no site data. This could happen when the sites list data were detected as not-fresh

mcsf added the [Status] In Progress label Apr 13, 2017

mcsf added the [Type] Question label Apr 13, 2017

mcsf assigned mcsf, ehg and mtias Apr 13, 2017

matticbot added the [Size] L Large sized issue label Apr 13, 2017

mcsf added Sites State labels Apr 13, 2017

mcsf mentioned this pull request Apr 14, 2017

Selectors: Add getSites() #13111

Merged

mcsf force-pushed the try/layout-query-sites branch 2 times, most recently from ef4f080 to 6f6fe46 Compare April 14, 2017 15:36

matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 14, 2017

gwwar reviewed Apr 14, 2017

View reviewed changes

ockham mentioned this pull request Apr 14, 2017

Selectors: Add areAllSitesSingleUser #13123

Merged

ockham reviewed Apr 14, 2017

View reviewed changes

mcsf force-pushed the try/layout-query-sites branch from 6f6fe46 to 14bbd1f Compare April 15, 2017 14:48

mcsf changed the title ~~Add QuerySites in Layout and shift from sitesFactory to Redux sites~~ Exploration: Top-down removal of SitesList, starting from Layout Apr 15, 2017

gwwar mentioned this pull request Apr 15, 2017

Framework: remove sitesList.getSelectedSite() usages #8726

Closed

76 tasks

mcsf force-pushed the try/layout-query-sites branch from 14bbd1f to 5ec97fa Compare April 17, 2017 16:42

mtias reviewed Apr 17, 2017

View reviewed changes

ockham mentioned this pull request Apr 18, 2017

Posts: Drop sites-list usage #13121

Merged

ockham reviewed Apr 18, 2017

View reviewed changes

mcsf force-pushed the try/layout-query-sites branch from 5ec97fa to 5fba283 Compare April 18, 2017 19:12

gwwar mentioned this pull request Apr 18, 2017

Framework: update lib/automated-transfer interface and remove usage #13191

Merged

mcsf added 12 commits May 9, 2017 14:25

Revert "Merge pull request #13160 from Automattic/update/site-is-cust…

0067812

…omizable" This reverts commit d180acc, reversing changes made to 78d8f29.

getSite: re-remove cyclical selector calls

d818a10

Stats: Controllers: Don't depend on sites-list

b9bed1c

* Import was kept, as `sites` is passed as props in one controller. To be fixed in a separate PR.

SitesDropdown: "fix" tests

722cc2d

Avoid duplicate selector call, use get

f14422c

state/lib/middleware: better naming, comments

380ae1f

siteSelection: fix redirs, wait for selected site's data

f4316f0

Keep SitesList's selected in sync with Redux

ed924e7

- :-( - Aims to fix regressions found, notably around flows in `/domains`

siteSelect: immediately redirectToPrimary if data ready

b07ce11

This mimics the behavior that used to be in place with `sites.initialized`. Without it, render jumps may happen as we default to a site-less view then correct to a site-specific one.

NavItem: fix propTypes (count)

123fe62

Component logic expects non-number `count`, e.g. when no site is selected. This eliminates React type warnings.

Invites: Pull sites from server upon accepting

698aa74

Before #13094, this behavior was ensured by the `acceptInvite` action creator, which dispatches old Flux actions that `lib/sites-list` listens to in order to call a `sync` on itself.

mcsf force-pushed the try/layout-query-sites branch from bb4ecb7 to 698aa74 Compare May 9, 2017 13:29

mcsf merged commit ef40e66 into master May 9, 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 May 9, 2017

hoverduck mentioned this pull request May 9, 2017

Invites: Remove extra step from accept invite flow #13801

Closed

blowery reviewed May 9, 2017

View reviewed changes

mcsf deleted the try/layout-query-sites branch May 9, 2017 14:38

This was referenced May 10, 2017

Framework: Remove sites-list usage from stats controller #13864

Merged

Notification Settings: Remove sites-list #13849

Merged

This was referenced May 13, 2017

Framework: Don't bind selectors to state in connect #14024

Closed

SiteSelector: Remove selector state binding #14028

Merged

gwwar mentioned this pull request Jun 7, 2017

Clean/site list/sites dropdown #9624

Closed

jsnajdr mentioned this pull request Dec 11, 2017

Plugins: remove sites-list and lib/site from plugins Flux store #20514

Merged

jsnajdr mentioned this pull request Jan 10, 2018

Site Selector: show placeholder only when site list is not available at all #21387

Merged

scinos mentioned this pull request Sep 30, 2020

Delete disabled tests #46064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exploration: Top-down removal of SitesList, starting from Layout #13094

Exploration: Top-down removal of SitesList, starting from Layout #13094

mcsf commented Apr 13, 2017 •

edited

Loading

matticbot commented Apr 13, 2017

gwwar Apr 14, 2017

mcsf Apr 15, 2017

ockham Apr 14, 2017

mcsf commented Apr 15, 2017 •

edited

Loading

mtias Apr 17, 2017

mtias Apr 19, 2017

gwwar Apr 19, 2017

mtias Apr 17, 2017

mtias Apr 17, 2017

mcsf Apr 18, 2017

ockham Apr 18, 2017

mcsf Apr 18, 2017

mtias Apr 18, 2017

gziolo Apr 25, 2017 •

edited

Loading

gwwar Apr 27, 2017

gwwar May 8, 2017

blowery May 9, 2017 •

edited

Loading

blowery May 9, 2017

mcsf May 9, 2017

Exploration: Top-down removal of SitesList, starting from Layout #13094

Exploration: Top-down removal of SitesList, starting from Layout #13094

Conversation

mcsf commented Apr 13, 2017 • edited Loading

Do not merge Yay!

matticbot commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Apr 15, 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

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

gziolo Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsf commented Apr 13, 2017 •

edited

Loading

mcsf commented Apr 15, 2017 •

edited

Loading

gziolo Apr 25, 2017 •

edited

Loading

blowery May 9, 2017 •

edited

Loading