Refactor: Site Settings Export components

[jordwest]

The components under client/my-sites/exporter had grown over time and the structure was becoming confusing. This arranges the export and guided transfer component files into a heirarchy more closely matching the way they are displayed on the page, and improves separation of concerns.

Prior to this change, the export component and guided transfer state were intermingled with a long list of props. This separates the two and adds Redux connectors where necessary to ensure each card only has access to the state it requires.

Before

• exporter
  • index.jsx
  • exporter.jsx
  • advanced-settings.jsx
  • guided-transfer-options.jsx
  • guided-transfer-details.jsx
  • guided-transfer-in-progress.jsx
  • spinner-button.jsx
  • style.scss

After

• exporter
  • export-card
    • index.jsx
    • advanced-settings.jsx
    • post-type-options.jsx
    • select.jsx
    • spinner-button.jsx
  • guided-transfer-card
    • index.jsx
    • in-progress.jsx
  • index.jsx
  • notices.jsx
  • style.scss

How to test

1. Go to My Sites > Settings > Export
2. Select custom export content, then click Export. Ensure the export completes as usual and a success notice appears.
3. Click Purchase a Guided Transfer and you should be redirected to the host selection screen (which is not affected by this PR)

Test live: https://calypso.live/?branch=update/exporter/reorganize-components

[omarjackman]

I feel like you can get rid of these three function variables and the need to get this.props.siteId if you add them to mapDispatchToProps and just pass them as this.props.exportAll, this.props.exportSelectedItems and this.props.fetchStatus. Then mapDispatchToProps would look something like:

function dispatchStartExportEvent( dispatch, siteId, options ) {
    analytics.tracks.recordEvent( 'calypso_export_start_button_click', {
        scope: ( options && options.exportAll === false ) ? 'selected' : 'all'
    } );
    dispatch( startExport( siteId, options ) );
}

function mapDispatchToProps( dispatch, ownProps ) {
    const { siteId } = ownProps;
    const exportStatusFetch = flowRight( dispatch, exportStatusFetch );
    return {
        advancedSettingsFetch: flowRight( dispatch, advancedSettingsFetch ),
        setPostType: flowRight( dispatch, setPostType ),
        exportAll: () => dispatchStartExportEvent( dispatch, siteId ),
        exportSelectedItems: () => dispatchStartExportEvent( dispatch, siteId, { exportAll: false } ),
        fetchStatus: () => exportStatusFetch( siteId ),
    };
}

[jordwest]

I like this idea and agree the functions seemed superfluous, but went a slightly different direction. In 1449623 I moved the analytics recording out from the connect statement and into the component. Generally I prefer to keep the connect to the minimum required logic to connect what's needed with Redux. That way, anything non-Redux related isn't dependant on Redux logic and can live inside the component.

Btw, in the example above siteId won't actually be passed in via ownProps, as connect won't include props from the mapStateToProps. It's possible to add a third parameter to the connect statement (mergeProps) which combines the two, but generally it makes more sense to move the logic into the React component, or, as In this case, move the siteId to the parent component so that it does get passed in via ownProps.

[omarjackman]

@jordwest I commented on some of the code though I know some of it was probably already there and you just moved it around. I didn't actually test the branch yet but I will a bit later today and comment on my experience if I find anything weird.

[jordwest]

Thanks for taking a look @omarjackman! I've made some changes and left inline replies

[omarjackman]

@jordwest your changes look good to me but I'd like to see at least one other review on client/my-sites/exporter/export-card/index.jsx. Maybe @dmsnell or @mtias wouldn't mind giving a quick review :)

[dmsnell]

we're now importing directly from lodash due to a babel transform that pulls out the modular components for us, so you can now comfortably do this…

import {
    flowRight
} from 'lodash';

[jordwest]

Fixed in ea32d98

[dmsnell]

nice changes @jordwest - I don't know about you, but I like the way that analytics calling looks now better than mixed into the component.

[jordwest]

Thanks for the tip @dmsnell. I didn't even realize we had analytics in Redux, and I like the composability :)

Currently it seems to have broken exporting though, will look into that and then mark it for review again

[jordwest]

Thanks for walking through the issues with me @dmsnell. Fixes are in 39cde7e and a738f64, and I've left replies on your inline comments.

As discussed in slack, 39cde7e makes the small tweak to the joinAnalytics/withAnalytics

[omarjackman]

@jordwest This looks really good. Unless @dmsnell has any objections I say LGTM