Wp-calypso: Framework: remove sitesList.getSelectedSite() usages

[gwwar]

As part of our effort to remove SitesList, let's update usages of sitesList.getSelectedSite() with the getSelectedSiteId selector

import { getSelectedSiteId } from 'state/ui/selectors'; 

export default connect(
    function( state ) {
        return {
            selectedSiteId: getSelectedSiteId( state )
        };
    }
)( MyExampleComponent );

• [x] client/boot.js (@seear #8879)
• [x] client/components/data/domain-management/index.js (@dmsnell, #8966)
• [x] client/components/data/domain-management/dns (@dmsnell, #9044)
• [x] client/components/data/domain-management/email (@dmsnell, #8760)
• [x] client/components/data/domain-management/email-forwarding (@dmsnell, #9045)
• [x] client/components/data/domain-management/name-servers (@dmsnell, #9047)
• [x] client/components/data/domain-management/primary-domain (@dmsnell, #9049)
• [x] client/components/data/domain-management/site-redirect ( @mtias, #13261 )
• [x] client/components/data/domain-management/name-servers (done)
• [x] client/components/data/domain-management/transfer ( @mtias, #13261 )
• [x] client/components/data/domain-management/who-is ( @mtias, #13261 )
• [x] client/components/email-verification ( @gwwar, #13098 )
• [x] client/components/plans/premium-popover ( @gwwar, #8687 )
• [x] client/components/sites-dropdown ( @mtias, #13094 / @ockham, #13293 )
• [x] client/components/tinymce/plugins/media (@youknowriad, #8448)
• [x] client/components/tinymce/plugins/media/drop-zone.jsx (@ehg #13230)
• [x] client/components/tinymce/plugins/wpcom-view (@mcsf, #13481)
• [x] client/components/tinymce/plugins/wpcom-view/views/embed (@jorgefilipecosta #15532)
• [x] client/components/upgrades/goggle-apps ( @mtias, #13263 )
• [x] client/layout/masterbar (@seear #8807 )
• [x] client/lib/abtest ( @gwwar, #8931 )
• [x] client/lib/analytics ( @gwwar, #13106 )
• [x] client/lib/automated-transfer (@mcsf, @gwwar, #13139, #13191, #13200)
• [x] client/lib/cart/store ( @gwwar, #13124, #13154 )
• [x] client/lib/desktop ( @gwwar, #13204 )
• [x] client/lib/keyboard-shortcuts ( @gwwar, #13206 )
• [x] client/lib/menu-data (@ockham #12975)
• [x] client/lib/menu-data/test/lib (@ockham #12975)
• [x] client/lib/olark ( @omarjackman #12432)
• [x] client/lib/plans ( @ockham, @gwwar #9635, #13252 )
• [x] client/lib/plugins
• [x] client/lib/posts ( @ockham #13121 )
• [x] client/lib/site-stats-stick-tab (doesn't exist)
• [x] client/lib/upgrades/actions (@gziolo #13304)
• [x] client/me/purchases ( @mtias, #13247 )
• [x] client/my-sites/controller.js (@mcsf, #13094)
• [x] client/my-sites/ads (@tyxla, #13119)
• [x] client/my-sites/current-site ( @mtias, #13094 )
• [x] client/my-sites/drafts ( @gwwar, #13107 )
• [x] client/my-sites/media ( @gwwar, #13109 )
• [x] client/my-sites/menus (@ockham #12975)
• [x] client/my-sites/pages (@obenland #10253)
• [x] client/my-sites/people (@gwwar #13161 )
• [x] client/my-sites/plans ( @tug #8159 )
• [x] client/my-sites/plugins (@tyxla, #9179, #9378)
• [x] client/my-sites/plugins/disconnect-jetpack (@tyxla, #9175)
• [x] client/my-sites/plugins/jetpack-plugins-setup (@tyxla, #9089)
• [x] client/my-sites/plugins/plugins-browser (@jorgefilipecosta #15765)
• [x] client/my-sites/plugins/plugins-list (@jorgefilipecosta #15743)
• [x] client/my-sites/posts (@ockham, #13121)
• [x] client/my-sites/sharing (@ehg #13223,@lamosty #20192)
• [x] client/my-sites/sharing/connections (@obenland #8991)
• [x] client/my-sites/sharing/connections/services (@obenland #8991)
• [x] client/my-sites/sidebar (@ockham, #13196)
• [x] client/my-sites/sidebar-navigation (@ockham #13140)
• [x] client/my-sites/site-settings ( @oskosk, #13300 )
• [x] client/my-sites/site-settings/delete-site ( @oskosk, #13468 )
• [x] client/my-sites/site-settings/seo-settings ( @gwwar, #13258 )
• [x] client/my-sites/site-settings/start-over ( @tyxla #13215 )
• [x] client/my-sites/site-settings/theme-setup ( @tyxla #13215 )
• [x] client/my-sites/site-settings/traffic ( @gwwar, #13258 )
• [x] client/my-sites/stats/follows ( @timmyc #8765 )
• [x] client/my-sites/stats/summary ( @timmyc #8766 )
• [x] client/my-sites/themes (@ockham #12124)
• [x] client/my-sites/upgrade-nudge ( @ockham #9635 )
• [x] client/my-sites/upgrades ( @gwwar, #13256 )
• [x] client/my-sites/upgrades/checkout ( no usages )
• [x] client/my-sites/upgrades/domain-management (@gziolo #13361)
• [x] client/my-sites/upgrades/domain-search (@tug, #12941)
• [x] client/my-sites/upgrades/map-domain (@nb & @Tug, #12804 & #12943)
• [x] client/post-editor ( @gwwar, #13132 )
• [x] client/post-editor/editor-title (@youknowriad, #8814)
• [x] client/post-editor/media-modal (@youknowriad, #8448)
• [x] client/post-editor/media-modal/test (@youknowriad, #8448)
• [x] client/signup/jetpack-connect (@tyxla, #8976)
• [x] client/vip (folder removed)

[spen]

I'm keen to take a pop at a couple of these!
I'm thinking of getting started with the sidebar files (Just picked at random at this point, happy to pick something else if something is more of a priority).
It looks like they could do with a general spruce up to get them to the current standards so I'm thinking to kick off with a PR that just aims to update the code style, then shoot out a second PR with the actual changes - is this a good / worthwhile approach?

TIA guys :)

[gwwar]

Thanks @spentay Some of these are a bit of a 🐰 🕳, so please don't hesitate to ask for help.

So I'm thinking to kick off with a PR that just aims to update the code style, then shoot out a second PR with the actual changes

That sounds like a fine approach. We've also been experimenting with doing the minimum required to remove sites list usages, but we can easily fall into wanting to refactor everything.

[brunoscopelliti]

Hi @gwwar,
I'd like to help here;
I'm going to start from client/lib/keyboard-shortcuts... if I'll have any doubts, I count on your help :)

[gwwar]

Sounds great @brunoscopelliti just remember to ping me on any PR or questions. I haven't had as much time lately to browse the open PR list, but I try to catch up with direct pings.

[brunoscopelliti]

Hi @gwwar,
sorry for the missing of feedback; I was ill for a few days.

This morning I had the time to take a look at client/lib/keyboard-shortcuts and before start messing with the code, I'd like to ask you a few questions, just to be sure that I am on the right track.

I’ve just noticed that I’ve not picked a regular component, but a lib utility. In this case using connect does not seem to be possible. Am I wrong?

The only way to make GlobalShortcuts aware of the selectedSiteId I’ve thought, is to explicitly pass the store to it.

// boot/index.js
- require( 'lib/keyboard-shortcuts/global' )( sites );
+ require( 'lib/keyboard-shortcuts/global' )( reduxStore );

... and then something like:

// lib/keyboard-shortcuts/global
+ import { getSelectedSite } from 'state/ui/selectors'; 

// ...

GlobalShortcuts.prototype.goToStats = function() {
  - var site = this.sites.getSelectedSite();
  + var site = getSelectedSite( this.store.getState() );
  // ...

Do you think this could be a suitable solution?
My primary concern is about passing the redux store around... I am not sure is a good practice, can you reassure me here?

There are also another couples of things (eg. exporting a constructor function vs default function + named exports) I’d like to discuss with you about this particular lib; but we could talk about this later when you’ll review the PR.

[gwwar]

Whoops @brunoscopelliti, I didn't notice you claimed one of the lib dirs. We're still working out approaches for these. So far we're trying to either remove the data dependency, update the signature to pass the site data, or rewrite some of these into components like we did for <DocumentHead />.

In this particular case, it might make sense to fire redux events on a matching key sequence, and add redux middleware for handling those keypresses. This would be a much larger refactor though, so perhaps you could work on one of the components while we work out a better strategy.

[brunoscopelliti]

No problem @gwwar .
Meanwhile, I'll try to work on client/components/sites-dropdown.

[brunoscopelliti]

Hi @gwwar,

I've picked client/components/sites-dropdown.
Currently, you could track the work I'm doing here.

In this case we can't use getSelectedSite because the component considers as initially selected the user's primary site... the component receives this info as selected prop; I used getSite instead.
However I noticed this prop is valorized sometimes with the siteId of the primary site (eg. me/account/main.jsx), and in other cases with the siteSlug (eg. me/help/help-contact-form/index.jsx).

This also impacts how we valorize the selected component state property, and in the end cause this warning in console:

Warning: Failed prop type: Invalid prop `selected` of type `number` supplied to `SiteSelector`, expected `string`.

Imho using the same prop for two different data is ambiguous... Is there a particular reason for this? Can we reinforce the use of siteId?

ps. Do you prefer that I open another issue where continue this discussion (I feel I am bloating up this thread)?

[gwwar]

Do you prefer that I open another issue where continue this discussion?

@brunoscopelliti sure, let's continue the discussion in an in-progress PR. Just be sure to ping me on it when you create it

[omarjackman]

I removed sitesList.getSelectedSite() from lib/olark in https://github.com/Automattic/wp-calypso/pull/12432

[ehg]

Added the unclaimed ones to https://github.com/Automattic/wp-calypso/projects/3 for better visibility, since we have so little left :)

[lamosty]

Closing the issue as nobody reads from sites-list anymore!