-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Exporter: WP.com API integration #1359
Conversation
739e97f
to
bece6f9
Compare
b73c88d
to
802b7d4
Compare
5fef3da
to
2b09f01
Compare
6a6b3ac
to
1e6d842
Compare
1e6d842
to
e3add36
Compare
@@ -34,25 +34,41 @@ export default React.createClass( { | |||
pages: this.translate( 'Pages' ), | |||
feedback: this.translate( 'Feedback' ) | |||
}; | |||
const defaultLabels = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be translated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed that in a past refactor. Fixed
07e616c
to
17badb2
Compare
Thanks for the review @dllh - I've fixed up those issues and rebased to fix the merge conflict |
It's possible to navigate to Update: To clarify re Jetpack, it's possible to manually specify a url and get a form that doesn't work. For import, if you do that, you get a slightly different view that directs you over to wp-admin for the site to do the export. |
This change completes the exporter, adding the API calls to communicate with wp.com
This refactors the Exporter so that the `site` prop is no longer needed. Instead the current site is retrieved from the Redux store using the `getSelectedSite` selector in state/lib/ui.js. The site was the only remaining external prop (excluding the Redux store) that was depended upon by the Exporter prior to this change.
d2b87e2
to
4c985d0
Compare
this.wpcom.req.get( { | ||
apiVersion: '1.1', | ||
path: `/sites/${ siteId }/exports/settings` | ||
}, fn ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you return these calls, then your code can take advantage of that by inspecting the request object.
additionally, if you return them and don't use a callback, the wpcom.js
library will return a Promise that resolves on the successful response from the API and rejects on error
Undocumented.prototype.getExportSettings = function( siteId ) {
return this.wpcom.req.get( {
apiVersion: '1.1',
path: `/sites/${ siteId }/exports/settings`
} );
}
//~~~ another file ~~~~
wpcom.getExportSettings( siteId )
.then( receiveExportSettings )
.catch( apiFailure );
@jordwest this is a pretty big PR. Since this isn't public yet, I suppose it's not too bad, but it would be really cool if it were split up into smaller functional blocks for testing purposes. In general, there's a lot of use of passing the entire global state around. I'm not sure this is really needed or beneficial. That exposes many of the smaller functions to a wider range of possible issues. If instead of sending the state object around, we only passed the data that they need, it would be more clear what they are doing. When thinking about testing too, it involves less setup for the test since we can mock up some related object without having to know the entire state structure to make sure we build the appropriate container for that mock object. I left many comments and would be happy to review this again after you respond! |
Thanks for the in-depth (as always!) review @dmsnell. I've made some changes based on your feedback, but I'm now working on splitting this up into smaller PRs so the modifications will all be incorporated in those |
This PR is being split into smaller PRs:
Awaiting #500 => #1126This change completes the exporter, adding the API calls to communicate with wp.com
How to test
Export All
Export Specific Content
Jetpack sites
Jetpack sites still display the export interface, but should instead show a message with a button link to wp-admin. Since export is still behind a feature flag, this will be covered in #77 under a separate PR.
Mobile
![export_mobile_flow](https://cloud.githubusercontent.com/assets/416133/12288388/ce512cac-ba1f-11e5-8f85-a4973dd164d0.gif)
If you are set up with a local dev proxy, please try it out on mobile. It would be good to get an idea of the flow on Android. I've tested on iOS 9:
Todo
site
prop to theExporter
componentprepareExportRequest( ... )
in state/site-settings/exporter/selectors.js)