Framework: Remove sites-list from ProfileLinksAddWordPress #14758

tyxla · 2017-06-05T11:57:46Z

This PR removes the sites-list usage from the ProfileLinksAddWordPress component. It also ES6ifies the component and introduces a straightforward getPublicSites selector that we need in order to allow only public sites to be added as profile links.

To test:

Checkout this branch
Go to http://calypso.localhost:3000/me
In the "Profile Links" section, click the "Add" button and from the dropdown menu click "Add WordPress Site"
Verify the profile link addition works properly and there are no regressions.
Verify the tests of the new selector pass:

npm run test-client client/state/selectors/test/get-public-sites.js

matticbot · 2017-06-05T11:57:47Z

Test live: https://calypso.live/?branch=remove/sites-list-profile-links

a8ci18n · 2017-06-05T12:03:08Z

client/me/profile-links-add-wordpress/index.jsx

 		return (
 			<form>
 				<p>
 					{
-						this.translate(
+						translate(
 							'You have no public sites on WordPress.com yet, but if you like you ' +


Hi! I've found a possible matching string that has already been translated 21 times:
translate( 'You have no public sites on WordPress.com yet, but if you like you can create one now. You may also add self-hosted WordPress sites here after installing {{jetpackLink}}Jetpack{{/jetpackLink}} on them.' ) ES Score: 18

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).

But I'm not changing the string, @a8ci18n?

a8ci18n · 2017-06-05T12:03:09Z

client/me/profile-links-add-wordpress/index.jsx

 				>
-					{ this.translate( 'Add Site', 'Add Sites', { count: checkedCount } ) }
+					{ translate( 'Add Site', 'Add Sites', { count: checkedCount } ) }


😞 85 existing translations will be lost with this change.

But I'm not changing the string, @a8ci18n?

gwwar · 2017-06-05T19:06:19Z

client/state/selectors/get-public-sites.js

+ * @param {Object} state  Global state tree
+ * @return {Array}        Site objects
+ */
+export default function getPublicSites( state ) {


Looks like this is probably worth memoizing with createSelector

Good point, done in dc84197.

gwwar · 2017-06-05T20:29:20Z

client/me/profile-links-add-wordpress/index.jsx

+
+export default connect(
+	( state ) => ( {
+		sites: keyBy( getSites( state ), 'ID' ),


Do you think we can cut down data needs here? I believe @mcsf ran into a similar issue in #14026

Agreed. There might be a way.

this.props.sites seems to only be used to obtain name and URL in preparation of the call to this.props.userProfileLinks.addProfileLinks. In that case, I would define an action creator:

// pseudo-code-ish const addProfileLinks = ( inputs ) => ( dispatch, getState ) => { const links = inputs .filter( isSiteInput ) .map( toSiteId ) .map( getSite( getState(), _ ) ) .map( pick( _, [ 'name', 'URL' ] ) ); if ( links.length ) userProfileLinks.addProfileLinks( links ); };

If not, at the very least you should cache this selection, since keyBy could potentially process a large number of sites and, more importantly, will always return a new reference.

I've gone with your suggestion @mcsf, using the inline action-y approach in f79b512.

mcsf · 2017-06-06T10:44:42Z

client/me/profile-links-add-wordpress/index.jsx

+						title: site.name,
+						value: site.URL
+					} );
+				}


[obviously besides the scope of this PR] May I just say that I hate that we still have these data libraries like UserProfileLinks? :/ It took me a while to realize why we were duplicating site state into links, that this is actually an action "dispatch" in disguise.

Yeah, I agree completely. It's a very obsolete pattern that we should consider refactoring in a future PR.

mcsf · 2017-06-06T10:56:49Z

client/me/profile-links-add-wordpress/index.jsx

@@ -157,84 +157,94 @@ export default React.createClass( {
 						<Site
 							site={ site }
 							indicator={ false }
-							onSelect={ this.onSelect.bind( this, inputName ) } />
+							onSelect={ this.onSelect( inputName ) } />


Note that this is still an instance of function binding within render. No cheating! 😄

The core problem is that we want to assign/bind (not necessarily in the Function#bind sense) a particular site/input to an event handler (onSelect). There are several ways to deal with it. One, which might make the most sense here, is to refactor the <li> node into a component — e.g.:

class AddableSite extends PureComponent { inputName = `site-${ site.ID }` onSelect = this.props.onSelect.bind( null, inputName ) render() { return <li>…<Site site={ site } onSelect={ this.onSelect } … />…</li>; } } // … renderAddableSites() { return this.props.publicSites.map( ( site ) => this.props.userProfileLinks.isSiteInProfileLinks( site ) ? null : <AddableSite site={ site } />;

Good point there, addressed in 8613691.

mcsf · 2017-06-06T11:00:11Z

client/me/profile-links-add-wordpress/index.jsx

+
+export default connect(
+	( state ) => ( {
+		sites: keyBy( getSites( state ), 'ID' ),


Agreed. There might be a way.

this.props.sites seems to only be used to obtain name and URL in preparation of the call to this.props.userProfileLinks.addProfileLinks. In that case, I would define an action creator:

// pseudo-code-ish const addProfileLinks = ( inputs ) => ( dispatch, getState ) => { const links = inputs .filter( isSiteInput ) .map( toSiteId ) .map( getSite( getState(), _ ) ) .map( pick( _, [ 'name', 'URL' ] ) ); if ( links.length ) userProfileLinks.addProfileLinks( links ); };

If not, at the very least you should cache this selection, since keyBy could potentially process a large number of sites and, more importantly, will always return a new reference.

mcsf · 2017-06-06T11:13:18Z

client/state/selectors/get-public-sites.js

+ * @param {Object} state  Global state tree
+ * @return {Array}        Site objects
+ */
+export default function getPublicSites( state ) {


tyxla · 2017-06-27T16:24:22Z

@mcsf @gwwar thanks for the great feedback! I've addressed it all, feel free to give it another shot. Thanks!

gwwar · 2017-06-28T22:17:03Z

client/me/profile-links-add-wordpress/site.jsx

+		checked: false,
+	};
+
+	inputName = `site-${ this.props.site.ID }`;


Does this update properly outside of render? I feel like this won't be correct if we pass another site/onSelect to this component

Does this work?

onSelect = ( event ) => { this.props.onSelect( event, `site-${ this.props.site.ID }` ); };

That shouldn't be a problem, because we're mounting one ProfileLinksAddWordPressSite instance for each site anyway. But it's a nice improvement, so I've addressed in f65e2af.

gwwar

Other than the note I left, things look good 👍

tyxla · 2017-06-29T09:58:40Z

Thanks! I've addressed your recommendation @gwwar, do you have any other tips before we 🚢 ?

gwwar · 2017-06-29T21:31:38Z

tyxla added Framework [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. Sites [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Jun 5, 2017

tyxla self-assigned this Jun 5, 2017

tyxla requested a review from ockham June 5, 2017 11:57

tyxla requested review from mcsf, ehg, mtias and gwwar June 5, 2017 11:57

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Jun 5, 2017

mtias mentioned this pull request Jun 5, 2017

Framework: remove rest of sites-list uses #13279

Closed

23 tasks

a8ci18n reviewed Jun 5, 2017

View reviewed changes

gwwar reviewed Jun 5, 2017

View reviewed changes

mcsf reviewed Jun 6, 2017

View reviewed changes

tyxla 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 6, 2017

tyxla force-pushed the remove/sites-list-profile-links branch from 67b36f1 to 901bdac Compare June 27, 2017 11:43

tyxla 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 27, 2017

gwwar reviewed Jun 28, 2017

View reviewed changes

gwwar approved these changes Jun 28, 2017

View reviewed changes

tyxla 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 30, 2017

tyxla added 8 commits June 30, 2017 10:35

State: Introduce getPublicSites selector

2546f44

Me: Remove eventRecorder mixin from ProfileLinksAddWordPress

149b57b

Me: Refactor ProfileLinksAddWordPress to an ES6 class

cb42b02

Me: Remove sites-list from ProfileLinksAddWordPress

33588c1

State: Memoize getPublicSites selector

2a7b1f5

Me: Move add WordPress link site to a separate component

1ae30b6

Me: Optimize adding of WordPress profile links

1bbd512

Me: Improve ProfileLinksAddWordPressSite site selection

debc1c5

tyxla force-pushed the remove/sites-list-profile-links branch from f65e2af to debc1c5 Compare June 30, 2017 07:41

tyxla merged commit dbd79f9 into master Jun 30, 2017

tyxla deleted the remove/sites-list-profile-links branch June 30, 2017 08:01

tyxla removed the [Status] Ready to Merge label Jun 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Remove sites-list from ProfileLinksAddWordPress #14758

Framework: Remove sites-list from ProfileLinksAddWordPress #14758

tyxla commented Jun 5, 2017

matticbot commented Jun 5, 2017

a8ci18n Jun 5, 2017

tyxla Jun 5, 2017

a8ci18n Jun 5, 2017

tyxla Jun 5, 2017

gwwar Jun 5, 2017

mcsf Jun 6, 2017

tyxla Jun 27, 2017

gwwar Jun 5, 2017

mcsf Jun 6, 2017

tyxla Jun 27, 2017

mcsf Jun 6, 2017

tyxla Jun 27, 2017 •

edited

Loading

mcsf Jun 6, 2017

tyxla Jun 27, 2017

mcsf Jun 6, 2017

mcsf Jun 6, 2017

tyxla commented Jun 27, 2017

gwwar Jun 28, 2017 •

edited

Loading

gwwar Jun 28, 2017

tyxla Jun 29, 2017

gwwar left a comment

tyxla commented Jun 29, 2017 •

edited

Loading

gwwar commented Jun 29, 2017

Framework: Remove sites-list from ProfileLinksAddWordPress #14758

Framework: Remove sites-list from ProfileLinksAddWordPress #14758

Conversation

tyxla commented Jun 5, 2017

matticbot commented Jun 5, 2017

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

tyxla Jun 27, 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

tyxla commented Jun 27, 2017

gwwar Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwwar left a comment

Choose a reason for hiding this comment

tyxla commented Jun 29, 2017 • edited Loading

gwwar commented Jun 29, 2017

tyxla Jun 27, 2017 •

edited

Loading

gwwar Jun 28, 2017 •

edited

Loading

tyxla commented Jun 29, 2017 •

edited

Loading