Remove connections list module #8991

obenland · 2016-10-27T22:54:04Z

Moves Publicize connections to be based on Redux.

This includes creating, updating, and deleting connections. This will also modernize the existing components and probably restructure them in a more modular fashion.

See #5046.

matticbot · 2016-10-27T22:54:05Z

Test live: https://calypso.live/?branch=remove/connections-list-module

gwwar · 2016-10-28T15:34:05Z

client/state/sharing/keyring/reducer.js

-	[ PUBLICIZE_CONNECTION_CREATE ]: ( state, { ID, siteId } ) => ( { ...state, [ ID ]: {
-		...state[ ID ], sites: [ ...state[ ID ].sites, siteId.toString() ]
+	[ PUBLICIZE_CONNECTION_CREATE ]: ( state, { connection: { keyring_connection_ID: ID, site_ID } } ) => ( { ...state, [ ID ]: {
+		...state[ ID ], sites: [ ...state[ ID ].sites, site_ID.toString() ]


Is site_ID not always a fixed type?

This is also probably on the edge of readability for a one-liner. 👍 Thanks for the tests through, made what the reducer is doing pretty clear.

Maybe I should test what happens if we mix strings and integers here, not sure

I also think we should be very wary about destructuring more than a single level deep, as it becomes very difficult to read, especially with aliasing. Speaking of, naming should be id, not ID, per camelCasing guidelines.

Is site_ID not always a fixed type?

The /me/keyring-connections endpoint returns the site IDs as an array of strings. While user_ID gets treated with an absint() before being returned, site IDs do not.
Also see #9068 (comment)

gwwar · 2016-10-28T15:44:12Z

client/my-sites/sharing/connections/service.jsx

 	onDisconnectionSuccess: function() {
 		this.setState( { isDisconnecting: false } );
 		this.props.connections.off( 'destroy:error', this.onDisconnectionError );

-		notices.success( this.translate( 'The %(service)s account was successfully disconnected.', {
+		this.props.successNotice( this.props.translate( 'The %(service)s account was successfully disconnected.', {


If there's are appropriate redux action types to associate this with, adding these to notice middleware may make sense: https://github.com/Automattic/wp-calypso/blob/master/client/state/notices/middleware.js

Yes, I'll do that when porting update/delete actions over to redux. So far only Create is full redux.

gwwar · 2016-10-28T15:46:39Z

client/my-sites/sharing/connections/services-group.jsx

@@ -79,6 +63,7 @@ class SharingServicesGroup extends Component {

 export default connect(
 	( state, { type } ) => ( {
+		initialized: !! getSelectedSiteId( state ),


How does this relate to services being initialized?

If no site is selected, connecting services is moot. We can probably remove that, I assume it's from a time when we couldn't rely on having a selected site available?

gwwar · 2016-10-28T15:48:23Z

client/my-sites/sharing/connections/services/eventbrite.js

-var connectionsList = require( 'lib/connections-list' )(),
-	sites = require( 'lib/sites-list' )();
+import connectionsList from 'lib/connections-list';
+import sites from 'lib/sites-list';


Could we perhaps either connect this or update the functions to take in a site object instead of a siteId?

Can we connect modules that are not react components?

You can create a component that renders nothing like we do for QueryComponents or if it doesn't make sense we can update the interfaces to pass in a site.

gwwar · 2016-10-28T22:51:08Z

client/my-sites/sharing/connections/account-dialog.jsx


-	componentWillReceiveProps: function( nextProps ) {
+		this.onClose = this.onClose.bind( this );
+		this.onSelectedAccountChanged = this.onSelectedAccountChanged.bind( this );


Optional, but we can also do in the function def:

onSelectedAccountChanged = () => { }

aduth

This probably should have been many separate pull requests. My attentiveness wanes after the 20th file 😄

aduth · 2016-10-31T13:58:27Z

client/my-sites/sharing/connections/account-dialog-account.jsx

-} );
+}
+
+export default AccountDialogAccount;


Minor: You could include the default export on the same line as the class declaration itself:

export default class AccountDialogAccount extends Component {

Or is there a preference toward exporting bottom-of-file?

aduth · 2016-10-31T14:01:44Z

client/my-sites/sharing/connections/account-dialog.jsx

-			{ action: 'connect', label: this.translate( 'Connect' ), isPrimary: true }
-		];
+		if ( this.isSelectedAccountConflicting() ) {
+			this.props.warningNotice( this.props.translate( 'The connection marked {{icon/}} will be replaced with your selection.', {


Am I correct in understanding that this will change the conflict notice from being inline to being a global notice? Does that work well with the dialog being shown? Perhaps also worth a design review.

Ref: 3513-gh-calypso-pre-oss

Also, does including this in render run the risk of many notices being shown if renders occur unexpectedly?

aduth · 2016-10-31T14:03:47Z

client/my-sites/sharing/connections/account-dialog.jsx

+		const classes = classNames( 'account-dialog', {
+				'single-account': 1 === this.props.accounts.length
+			} ),
+			buttons = [


Might be more readable if this were a separate const declaration, not comma-separated, especially since both of these variables are multi-line. What do you think?

aduth · 2016-10-31T14:04:34Z

client/my-sites/sharing/connections/account-dialog.jsx

-		];
+		if ( this.isSelectedAccountConflicting() ) {
+			this.props.warningNotice( this.props.translate( 'The connection marked {{icon/}} will be replaced with your selection.', {
+				components: { icon: <span className="noticon noticon-warning" /> },


Not sure if it'd be considered in-scope, but noticons are deprecated in favor of <Gridicon /> and should ideally be updated.

aduth · 2016-10-31T14:08:25Z

client/my-sites/sharing/connections/connection.jsx

+			isSavingSitewide: false
+		};
+
+		this.toggleSitewideConnection = this.toggleSitewideConnection.bind( this );


I don't feel particularly strongly either way, but curious how you feel about class instance property initializers to simplify binding at the time of declaring the function?

toggleSitewideConnection = ( event ) => {

@gwwar had mentioned that to me as well. I like it, especially since it lets us avoid having to write out the constructor.

aduth · 2016-10-31T14:32:12Z

client/my-sites/sharing/connections/service.jsx

+import ServiceTip from './service-tip';
+import SocialLogo from 'components/social-logo';
+
+const SharingService = React.createClass( {


Any reason you chose not to Component-ify this one?

It still uses a mixin to monitor connections-list, I'm planning on doing it once all actions have moved over to using Redux

aduth · 2016-10-31T14:33:09Z

client/my-sites/sharing/connections/service.jsx

+		siteUserConnections: getSiteUserConnectionsForService(
+			state, getSelectedSiteId( state ), getCurrentUserId( state ), service.ID
+		),
+		user: getCurrentUser( state ),


Since we're only using user.ID, maybe better to pass userId: getCurrentUserId( state ) ?

aduth · 2016-10-31T14:34:39Z

client/my-sites/sharing/connections/service.jsx

 		}
 	},

+	addConnection: function( service = this.props.service, keyringConnectionId, externalUserId = false ) {
+		const _this = this,


This should be unnecessary since the popupMonitor event handler uses an arrow function (this still refers to the component instance).

aduth · 2016-10-31T14:37:27Z

client/post-editor/editor-sharing/accordion.jsx

@@ -49,7 +48,10 @@ const EditorSharingAccordion = React.createClass( {
 		const targeted = connections.filter( ( connection ) => {
 			return ! includes( skipped, connection.keyring_connection_ID );
 		} );
-		const targetedServices = serviceConnections.getServicesFromConnections( targeted );
+		const targetedServices = uniq( targeted.map( ( connection ) => ( {


I believe plain uniq is not what you want here:

https://lodash.com/docs/4.16.4#uniq

Instead, uniqBy is the one that lets you specify an iteratee:

https://lodash.com/docs/4.16.4#uniqBy

aduth · 2016-10-31T14:39:03Z

client/state/sharing/keyring/reducer.js

-	[ PUBLICIZE_CONNECTION_CREATE ]: ( state, { ID, siteId } ) => ( { ...state, [ ID ]: {
-		...state[ ID ], sites: [ ...state[ ID ].sites, siteId.toString() ]
+	[ PUBLICIZE_CONNECTION_CREATE ]: ( state, { connection: { keyring_connection_ID: ID, site_ID } } ) => ( { ...state, [ ID ]: {
+		...state[ ID ], sites: [ ...state[ ID ].sites, site_ID.toString() ]


I also think we should be very wary about destructuring more than a single level deep, as it becomes very difficult to read, especially with aliasing. Speaking of, naming should be id, not ID, per camelCasing guidelines.

See #8991 (comment)

@gwwar

* Update keyring reducer to use correct format. Turns out that publicize connection objects don’t actually look like I thought the look like. * Use full connection object when deleting a connection. Allows the keyring reducer to remove a connection based on the `keyring_connection_ID`. * Return all connections for a specified service. There may be more than one connection per service in the case of publicize services. * Introduce `getSiteUserConnectionsForService` Avoids having to filter the selector in the component that uses it. * Introduce `getAvailableExternalConnections` A selector for available external accounts for a service. * Simplify filtering of selector. * Extract action object to make it reusable * Handle update events for connections. * Update API to accept a connection object Simplifies its usage in components and allows us to pass its label to error messages. * Make keyring reducer easier to read. See #8991 (comment) * Introduce a selector for connections that can be removed by the current user. See https://github.com/Automattic/wp-calypso/pull/8991/files#r85749283 * Add missing function docs for new action creator. * Avoid using variable name previously defined in outer scope. Fixes an ESLint warning. * Remove unnecessary fallback value. `filter` always returns a new array. Props @gwwar. See #9068 (comment) * Limit variable destruction to one level deep. See #9068 (comment) * Move selector for external accounts to sharing directory. It’s a better place for the selector since it accesses both parts of sharing, `keyring` and `publicize`. It also returns more of a custom structure than either keyring or publicize connections. See #9068 (comment), #9068 (comment) * Remove unused reference to `additional_external_users`. See fddcca0. * Remove unused parts of test state. See fddcca0. * Simplify how removable connections are being retrieved. Checks capabilities based on the current user rather than the selected site. Props @gwwar. See https://github.com/Automattic/wp-calypso/pull/9068/files#r86043229.

@gwwar

Avoids having to define a constructor in 2/3 cases. Props @gwwar. See #8991 (comment), #8991 (comment)

See #8991 (comment)

@gwwar

Avoids having to define a constructor in 2/3 cases. Props @gwwar. See #8991 (comment), #8991 (comment)

See #8991 (comment)

Allows for easier handling of Eventbrite connections since it doesn’t use Publicize.

See #8991 (comment) 0

Avoids regression with previous bahavior. See #8991 (comment) 1

a8ci18n · 2017-03-01T20:13:54Z

client/my-sites/sharing/connections/service.jsx

 			}
 		} else {
 			// If an account wasn't selected from the dialog or the user cancels
 			// the connection, the dialog should simply close
-			this.props.connections.emit( 'create:error', { cancel: true } );
+			this.props.warningNotice( this.props.translate( 'The connection could not be made because no account was selected.', {


ℹ️ This string is probably long enough that it doesn't need a context. Are you sure you don't want to use a translator comment instead?

No need to use a context here. A translator comment is better suited ( i.e 'comment': 'Warning notice that appears If an account wasn't selected in the publicize/sharing dialog')

See #8991 (comment)

ockham · 2017-03-02T19:30:40Z

Means we can tick off a bunch of items in #8726? 🎉

obenland · 2017-03-02T19:59:13Z

Oh, good point! I checked off two items that were fixed by this PR

See #8991

artpi · 2017-03-03T13:39:51Z

Whoa, nice to see that in!

See #8991

obenland added [Status] In Progress State labels Oct 27, 2016

obenland self-assigned this Oct 27, 2016

gwwar reviewed Oct 28, 2016

View reviewed changes

obenland force-pushed the remove/connections-list-module branch from 0be2ea2 to c7b2545 Compare October 28, 2016 15:51

aduth mentioned this pull request Oct 28, 2016

Publicize: point wp-admin users to WP.com calypso sharing page instead Automattic/jetpack#3258

Closed

gwwar reviewed Oct 28, 2016

View reviewed changes

aduth reviewed Oct 31, 2016

View reviewed changes

obenland force-pushed the remove/connections-list-module branch from efb050d to 7a3ece6 Compare November 1, 2016 18:04

obenland added a commit that referenced this pull request Nov 1, 2016

Make keyring reducer easier to read.

e9ed1f0

See #8991 (comment)

obenland mentioned this pull request Nov 1, 2016

State: Improve state handling for connections list #9068

Merged

obenland force-pushed the remove/connections-list-module branch from 7a3ece6 to 733fdb0 Compare November 2, 2016 23:07

obenland added a commit that referenced this pull request Nov 2, 2016

Use shorthand instead of binding in constructor.

bda7676

Avoids having to define a constructor in 2/3 cases. Props @gwwar. See #8991 (comment), #8991 (comment)

obenland added a commit that referenced this pull request Nov 2, 2016

Use user ID rather than selecting the entire user object.

733fdb0

See #8991 (comment)

obenland force-pushed the remove/connections-list-module branch from ada491a to ea75de9 Compare November 3, 2016 15:33

obenland force-pushed the remove/connections-list-module branch from ea75de9 to e851afc Compare November 3, 2016 15:35

obenland force-pushed the remove/connections-list-module branch from e851afc to 5fca738 Compare November 3, 2016 21:47

obenland added a commit that referenced this pull request Nov 3, 2016

Use shorthand instead of binding in constructor.

fa32ded

Avoids having to define a constructor in 2/3 cases. Props @gwwar. See #8991 (comment), #8991 (comment)

obenland added a commit that referenced this pull request Nov 3, 2016

Use user ID rather than selecting the entire user object.

5fca738

See #8991 (comment)

obenland mentioned this pull request Nov 7, 2016

Sharing: Update connections list to use ES6 #9188

Merged

obenland force-pushed the remove/connections-list-module branch from 5fca738 to bd3eb43 Compare November 8, 2016 23:09

obenland added 10 commits March 1, 2017 12:02

Use Redux to update shared connections

4b9f026

Use Redux to refresh connections.

f701b1e

Remove remaining ConnectionsList references.

5259950

Reorder methods to be more coherent.

29cd8e5

Convert SharingService to use ES6 class

9dd7439

Wait until connections are coming in before opening account dialog.

5aa15e5

Update state after cancelling the connection modal

ce4eaf9

Sharing: Create a custom Eventbrite service component

1870d58

Allows for easier handling of Eventbrite connections since it doesn’t use Publicize.

Limit amount of notices to one at a time

45b4c2e

See #8991 (comment) 0

Use warning rather than error notice

949e357

Avoids regression with previous bahavior. See #8991 (comment) 1

obenland force-pushed the remove/connections-list-module branch from f2fbe8d to 949e357 Compare March 1, 2017 20:02

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

a8ci18n reviewed Mar 1, 2017

View reviewed changes

Use a comment over context

1117385

See #8991 (comment)

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

obenland merged commit 2bc1668 into master Mar 2, 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 Mar 2, 2017

obenland deleted the remove/connections-list-module branch March 2, 2017 17:25

obenland mentioned this pull request Mar 2, 2017

Convert Sharing to use es6 #11724

Merged

obenland added a commit that referenced this pull request Mar 2, 2017

Remove connections list

dff7c9a

See #8991

obenland mentioned this pull request Mar 2, 2017

Sharing: Remove connections list library #11725

Merged

obenland added a commit that referenced this pull request Mar 3, 2017

Remove connections list (#11725)

063dcaf

See #8991

mtias added the [Feature] Sharing Features and settings for sharing posts across different platforms, including sharing buttons. label Mar 13, 2017

mtias mentioned this pull request Apr 3, 2017

Framework: Redux single data tree and query components #5046

Closed

aduth mentioned this pull request Sep 27, 2017

Sharing: Refactor connections data library #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove connections list module #8991

Remove connections list module #8991

obenland commented Oct 27, 2016

matticbot commented Oct 27, 2016

gwwar Oct 28, 2016

obenland Oct 28, 2016

aduth Oct 31, 2016

obenland Nov 2, 2016 •

edited

Loading

gwwar Oct 28, 2016

obenland Oct 28, 2016

gwwar Oct 28, 2016

obenland Oct 28, 2016

gwwar Oct 28, 2016

obenland Oct 28, 2016

gwwar Oct 28, 2016

gwwar Oct 28, 2016

aduth left a comment

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

obenland Nov 1, 2016

aduth Oct 31, 2016

obenland Nov 1, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

aduth Oct 31, 2016

a8ci18n Mar 1, 2017

yoavf Mar 1, 2017

ockham commented Mar 2, 2017 •

edited

Loading

obenland commented Mar 2, 2017

artpi commented Mar 3, 2017

Remove connections list module #8991

Remove connections list module #8991

Conversation

obenland commented Oct 27, 2016

matticbot commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obenland Nov 2, 2016 • 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

aduth left a 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

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

ockham commented Mar 2, 2017 • edited Loading

obenland commented Mar 2, 2017

artpi commented Mar 3, 2017

obenland Nov 2, 2016 •

edited

Loading

ockham commented Mar 2, 2017 •

edited

Loading