Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sharing: Implement Redux-based Publicize reducers and actions #338

Merged
merged 10 commits into from
Nov 24, 2015
Merged

Sharing: Implement Redux-based Publicize reducers and actions #338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
76bc322
Sharing: Implement Redux-based Publicize actions and stores
aduth Nov 20, 2015
5f5c917
Sharing: Integrate Redux-based Publicize to editor sharing accordion
aduth Nov 20, 2015
7d8b3fc
Framework: Allow store to be initialized on-demand
aduth Nov 20, 2015
e461dc5
Sharing: Extract connections filtering to separate helper function
aduth Nov 22, 2015
50586ea
Sharing: Move action types from global file to module-specific
aduth Nov 22, 2015
8e28055
Framework: Prevent reinitializing redux store per route change
aduth Nov 22, 2015
8bdd3d7
Sharing: Map subset of state tree in connect mapper
aduth Nov 23, 2015
9c39f0e
Sharing: Ensure connections exist when new props received
aduth Nov 24, 2015
1220fa3
Framework: Add license crediting for Redux packages
aduth Nov 24, 2015
c97533c
Sharing: Allow and guard against empty site
aduth Nov 24, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions CREDITS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,3 +1172,44 @@ Copyright © 2013 Habla, Inc. Habla Inc. (dba Olark)

Permission kindly granted by Ben Congleton, of Olark.
```

### https://github.com/rackt/redux/
#### shared/lib/create-redux-store
```text
The MIT License (MIT)

Copyright (c) 2015 Dan Abramov

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
```

### https://github.com/gaearon/redux-thunk
#### shared/lib/create-redux-store
```text
The MIT License (MIT)

Copyright (c) 2015 Dan Abramov

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
```

### https://github.com/rackt/react-redux
```text
The MIT License (MIT)

Copyright (c) 2015 Dan Abramov

Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
```
4 changes: 4 additions & 0 deletions client/boot/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var config = require( 'config' ),
touchDetect = require( 'lib/touch-detect' ),
accessibleFocus = require( 'lib/accessible-focus' ),
TitleStore = require( 'lib/screen-title/store' ),
createReduxStore = require( 'lib/create-redux-store' ),
// The following mixins require i18n content, so must be required after i18n is initialized
Layout,
LoggedOutLayout;
Expand Down Expand Up @@ -78,12 +79,15 @@ function init() {
}

function setUpContext( layout ) {
var reduxStore = createReduxStore();

// Pass the layout so that it is available to all page handlers
// and add query and hash objects onto context object
page( '*', function( context, next ) {
var parsed = url.parse( location.href, true );

context.layout = layout;
context.reduxStore = reduxStore;

// Break routing and do full page load for logout link in /me
if ( context.pathname === '/wp-login.php' ) {
Expand Down
29 changes: 24 additions & 5 deletions client/my-sites/sharing/connections/service-connections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,28 @@ module.exports = {
return this.filter( 'getConnectionStatus', serviceName, status, arguments );
},

/**
* Given a service name and array of connection objects, returns the subset
* of connections that are available for use by the current user.
*
* @param {string} serviceName The name of the service
* @param {Array} siteId Connection objects
* @return {Array} Connections available to user
*/
getConnectionsAvailableToCurrentUser: function( serviceName, connections ) {
var currentUser = user.get();

if ( ! currentUser ) {
return [];
}

return connections.filter( function( connection ) {
// Only include connections of the specified service, filtered by
// those owned by the current user or shared.
return connection.service === serviceName && ( connection.keyring_connection_user_ID === currentUser.ID || connection.shared );
} );
},

/**
* Given a service name and optional site ID, returns the available
* connections for the current user.
Expand All @@ -157,11 +179,8 @@ module.exports = {
siteId = null;
}

connections = connectionsList.get( siteId ).filter( function( connection ) {
// Only include connections of the specified service, filtered by
// those owned by the current user or shared.
return connection.service === serviceName && ( connection.keyring_connection_user_ID === currentUser.ID || connection.shared );
} );
connections = connectionsList.get( siteId );
connections = this.getConnectionsAvailableToCurrentUser( serviceName, connections );
} else {
connections = [];
}
Expand Down
15 changes: 9 additions & 6 deletions client/post-editor/controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
var React = require( 'react' ),
page = require( 'page' ),
ReduxProvider = require( 'react-redux' ).Provider,
startsWith = require( 'lodash/string/startsWith' ),
qs = require( 'querystring' );

Expand Down Expand Up @@ -37,12 +38,14 @@ function renderEditor( context, postType ) {

React.unmountComponentAtNode( document.getElementById( 'secondary' ) );
React.render(
React.createElement( PreferencesData, null,
React.createElement( PostEditor, {
sites: sites,
type: postType
} )
),
React.createElement( ReduxProvider, { store: context.reduxStore }, () => {
return React.createElement( PreferencesData, null,
React.createElement( PostEditor, {
sites: sites,
type: postType
} )
)
} ),
document.getElementById( 'primary' )
);
}
Expand Down
21 changes: 3 additions & 18 deletions client/post-editor/editor-drawer/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ var Accordion = require( 'components/accordion' ),
CategoryListData = require( 'components/data/category-list-data' ),
TagListData = require( 'components/data/tag-list-data' ),
FeaturedImage = require( 'post-editor/editor-featured-image' ),
SharingConnectionsData = require( 'components/data/sharing-connections-data' ),
SharingAccordion = require( 'post-editor/editor-sharing/accordion' ),
EditorSharingContainer = require( 'post-editor/editor-sharing/container' ),
FormTextarea = require( 'components/forms/form-textarea' ),
PostFormatsData = require( 'components/data/post-formats-data' ),
PostFormatsAccordion = require( 'post-editor/editor-post-formats/accordion' ),
Expand Down Expand Up @@ -114,23 +113,9 @@ var EditorDrawer = React.createClass( {
},

renderSharing: function() {
var element = (
<SharingAccordion
site={ this.props.site }
post={ this.props.post }
isNew={ this.props.isNew }
className="editor-drawer__accordion" />
return (
<EditorSharingContainer site={ this.props.site } />
);

if ( this.props.site ) {
element = (
<SharingConnectionsData siteId={ this.props.site.ID }>
{ element }
</SharingConnectionsData>
);
}

return element;
},

renderFeaturedImage: function() {
Expand Down
4 changes: 3 additions & 1 deletion client/post-editor/editor-sharing/accordion.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ module.exports = React.createClass( {
site: React.PropTypes.object,
post: React.PropTypes.object,
isNew: React.PropTypes.bool,
connections: React.PropTypes.array
connections: React.PropTypes.array,
fetchConnections: React.PropTypes.func
},

getSubtitle: function() {
Expand Down Expand Up @@ -116,6 +117,7 @@ module.exports = React.createClass( {
post={ this.props.post }
isNew={ this.props.isNew }
connections={ this.props.connections }
fetchConnections={ this.props.fetchConnections }
/>
) }
{ this.renderShortUrl() }
Expand Down
96 changes: 96 additions & 0 deletions client/post-editor/editor-sharing/container.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/**
* External dependencies
*/
import React, { Component, PropTypes } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think React is needed here? I think import { Component, PropTypes } from 'react'; would be sufficient.

If I am wrong, can you explain so that I can learn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think React is needed here?

While React is not referenced directly in the file, it's still necessary to import. When transpiled, the JSX markup is replaced with calls to React.createElement, so React must be declared.

<EditorSharingAccordion />
// ...is equivalent to...
React.createElement( EditorSharingAccordion );

The reason it is not marked as unused is because we use eslint-plugin-react in our ESLint configuration, which includes a jsx-uses-react rule:

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-uses-react.md

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the explanation

import { connect } from 'react-redux';

/**
* Internal dependencies
*/
import PostEditStore from 'lib/posts/post-edit-store';
import { fetchConnections } from 'lib/sharing/publicize/actions';
import { getConnectionsBySiteId, hasFetchedConnections } from 'lib/sharing/publicize/selectors';
import EditorSharingAccordion from './accordion';

class EditorSharingContainer extends Component {
constructor( props ) {
super( props );

// Set state
this.state = this.getState();

// Trigger connection fetch
this.ensureHasConnections();

// Bind legacy store update handler
this.boundUpdateState = this.updateState.bind( this );
PostEditStore.on( 'change', this.boundUpdateState );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? I'm guessing it has to do with the component not receiving post as a prop, but I don't entirely follow the flow here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? I'm guessing it has to do with the component not receiving post as a prop, but I don't entirely follow the flow here.

Correct. In an ideal future scenario, we'd have the edited post contained within the global state tree, and we could pass it through to the component via the react-redux selector. In the meantime, for interoperability with the Flux-styled stores, we need to manually bind/unbind change handlers to the store. By updating the state when the store changes, we ensure that render is called (since setState triggers a rerender) and the descendent component tree will receive the updated post object.

}

componentDidUpdate() {
this.ensureHasConnections();
}

componentWillUnmount() {
PostEditStore.off( 'change', this.boundUpdateState );
}

ensureHasConnections() {
if ( this.props.hasFetchedConnections ) {
return;
}

this.fetchConnections();
}

updateState() {
this.setState( this.getState() );
}

getState() {
return {
post: PostEditStore.get(),
isNew: PostEditStore.isNew()
};
}

fetchConnections() {
const { site, dispatch } = this.props;
if ( ! site ) {
return;
}

dispatch( fetchConnections( site.ID ) );
}

render() {
const { site, connections } = this.props;
const { post, isNew } = this.state;

return (
<EditorSharingAccordion
site={ site }
post={ post }
isNew={ isNew }
connections={ connections }
fetchConnections={ this.fetchConnections.bind( this ) }
className="editor-drawer__accordion" />
);
}
}

EditorSharingContainer.propTypes = {
site: PropTypes.object,
dispatch: PropTypes.func.isRequired,
hasFetchedConnections: PropTypes.bool,
connections: PropTypes.array
};

export default connect(
( state, props ) => {
return {
hasFetchedConnections: props.site && hasFetchedConnections( state, props.site.ID ),
connections: props.site ? getConnectionsBySiteId( state, props.site.ID ) : null
};
}
)( EditorSharingContainer );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the state mapping function always going to be the same? If not, why not (for my own education)? If so, it looks like this will be a common pattern going forward, so I think it makes sense to move this logic into a helper method like reduxify or similar.

I'm also finding it pretty confusing that state is used to refer to the state of the Redux store - this.props.state is a strange thing to see in the code. Perhaps calling it store or reduxState or something else would help?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the state mapping function always going to be the same?

This raises a good point, as it should depend on the data needs of the component. The react-redux docs examples may help to shed some light here. In this case, rather than injecting the entire state tree, I should probably leverage the existing selectors, specifying connections and hasFetchedConnections as the props passed by the connect mapping function.

I'm also finding it pretty confusing that state is used to refer to the state of the Redux store - this.props.state is a strange thing to see in the code.

"State" is the terminology used by Redux, though I do agree that it becomes a bit confusing in this example where I'm injecting a state prop into the the component. In consideration of the above, this will likely not be as big a concern if I choose to not pass the entire state tree in to the component, and instead pass only the subset of data needed by the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I remember why it wasn't simple to pass the subset of data -- since the selected site is not yet made available in the state tree, it's not possible to call hasFetchedConnections and getConnectionsBySiteId from the connect mapping function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, state maybe should be a reserved keyword

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that there is an alternate function signature for the connect mapping function, where the second parameter passes the props of the component, which should include site. Will see if I can use this to select based on the passed site prop.

10 changes: 6 additions & 4 deletions client/post-editor/editor-sharing/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ module.exports = React.createClass( {
site: React.PropTypes.object,
post: React.PropTypes.object,
isNew: React.PropTypes.bool,
connections: React.PropTypes.array
connections: React.PropTypes.array,
fetchConnections: React.PropTypes.func
},

isSharingButtonsEnabled() {
Expand Down Expand Up @@ -66,9 +67,10 @@ module.exports = React.createClass( {

return (
<PublicizeOptions
post={ this.props.post }
site={ this.props.site }
connections={ this.props.connections } />
post={ this.props.post }
site={ this.props.site }
connections={ this.props.connections }
fetchConnections={ this.props.fetchConnections } />
);
},

Expand Down
18 changes: 10 additions & 8 deletions client/post-editor/editor-sharing/publicize-options.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ var PublicizeMessage = require( './publicize-message' ),
PublicizeServices = require( './publicize-services' ),
paths = require( 'lib/paths' ),
PostMetadata = require( 'lib/post-metadata' ),
connections = require( 'lib/connections-list' )(),
PopupMonitor = require( 'lib/popup-monitor' ),
AddNewButton = require( 'components/add-new-button' ),
siteUtils = require( 'lib/site/utils' ),
Expand All @@ -29,7 +28,14 @@ module.exports = React.createClass( {
propTypes: {
site: React.PropTypes.object,
post: React.PropTypes.object,
connections: React.PropTypes.array
connections: React.PropTypes.array,
fetchConnections: React.PropTypes.func
},

getDefaultProps() {
return {
fetchConnections: () => {}
};
},

hasConnections: function() {
Expand Down Expand Up @@ -60,11 +66,7 @@ module.exports = React.createClass( {
}

this.connectionPopupMonitor.open( href );
this.connectionPopupMonitor.once( 'close', this.onNewConnectionPopupClosed );
},

onNewConnectionPopupClosed: function() {
connections.get( this.props.site.ID, { force: true } );
this.connectionPopupMonitor.once( 'close', this.props.fetchConnections );
},

newConnection: function() {
Expand Down Expand Up @@ -102,7 +104,7 @@ module.exports = React.createClass( {
// to connections previously returning a 400 error
this.props.site.once( 'change', () => {
if ( this.props.site.isModuleActive( 'publicize' ) ) {
connections.get( this.props.site.ID, { force: true } );
this.props.fetchConnections();
}
} );
},
Expand Down
5 changes: 4 additions & 1 deletion client/post-editor/editor-sharing/publicize-services.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ module.exports = React.createClass( {
},

renderConnections: function( serviceName ) {
var connections = serviceConnections.getConnections( serviceName, this.props.siteId );
const connections = serviceConnections.getConnectionsAvailableToCurrentUser(
serviceName,
this.props.connections
);

return connections.map( function( connection ) {
return (
Expand Down
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@
"raf": "2.0.4",
"react": "0.13.3",
"react-day-picker": "1.1.0",
"react-redux": "3.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you didn't go with 4.0.0 because we're still on react 0.13.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing you didn't go with 4.0.0 because we're still on react 0.13.x?

Yep, react-redux 4.x requires React 0.14. The biggest change between react-redux 3.x and 4.x that we'll need to account for is that the latter does not require the <Provider> child to be a callback function.

https://github.com/rackt/react-redux/releases/tag/v4.0.0

"react-tap-event-plugin": "0.1.6",
"redux": "3.0.4",
"redux-thunk": "1.0.0",
"rtlcss": "1.6.1",
"sanitize-html": "1.11.1",
"source-map": "0.1.39",
Expand Down
12 changes: 12 additions & 0 deletions shared/lib/create-redux-store/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
`create-redux-store`
====================

This module exports a function which, when invoked, returns an instance of a [Redux store](http://redux.js.org/docs/basics/Store.html). The store instance is intended to reflect the global state of the application, and runs dispatched actions against all reducers defined in [`reducers.js`](./reducers.js). To include a reducer in the global store, simply add the reducing function to the combined reducer exported by `reducers.js`.

## Usage

```js
import createReduxStore from 'lib/create-redux-store';

const store = createReduxStore();
```
Loading