Site Importer: Add Site Importer Beta to Calypso #23930

bisko · 2018-04-05T14:03:21Z

This PR adds a beta version of the Site Importer interface. Currently enabled only in development mode, while we figure out bugs and issues in the UX.

matticbot · 2018-04-05T14:03:22Z

Test live: https://calypso.live/?branch=add/site-importer-beta

jblz

How much testing have we done against the existing import methods with this diff?

jblz · 2018-04-05T16:01:25Z

client/my-sites/importer/site-importer/site-importer-input-pane.jsx

+	};
+
+	componentWillUnmount() {
+		window.clearInterval( this.randomizeTimer );


This interval doesn't seem used... Can it be pulled out?

It's a leftover from previous importer code and it's unused. Removed.

jblz · 2018-04-05T16:02:59Z

client/my-sites/importer/importer-site-importer.jsx

+
+	render() {
+		importerData.description = this.props.translate(
+			'Import posts, pages, and media ' + 'from your existing site!'


why is this concatenated? can it be joined w/o exceeding the line limit?

Something got lost somewhere in updating the strings :) Thanks for noticing! Updated.

jblz · 2018-04-05T16:04:55Z

client/my-sites/importer/importer-site-importer.jsx

+ * Module variables
+ */
+const importerData = {
+	title: 'Site Importer',


Can we add a (Beta) label to this in the meantime so folks in dev don't expect it to work 100% in the dev env?

Yes, great suggestion. Added the label.

bisko · 2018-04-05T16:23:11Z

How much testing have we done against the existing import methods with this diff?

This method doesn't touch on current imports code. To be on the safe side I tested it with WordPress imports and it seems to be working properly.

There's an ongoing issue with Medium imports at the moment - #17631 , which is not related to the current PR.

marekhrabe · 2018-04-06T14:39:59Z

client/my-sites/importer/site-importer/site-importer-input-pane.jsx

+		siteURLInput: '',
+	};
+
+	// TODO This can be improved if we move to Redux.


This fn looks awful with those defers but agreed that we can handle it better once switching to redux.

marekhrabe · 2018-04-09T13:23:04Z

The only question that's left for me is whether to use translate() or not. This PR mixes both approaches and I think we should settle on one before merging. Other than that decision, this PR looks good to me to be 🚢soon!

spen

Re: the translate usage, I'd err on the side of not translating until finalised. For strings that are certainly translated already (translate( 'no' ) for instance) that's not a problem as we're not making work for translators that may/may not be staying.

spen · 2018-04-05T20:56:35Z

client/my-sites/importer/importer-site-importer.jsx

+/**
+ * Module variables
+ */
+const importerData = {


Is there specific benefit to creating the importerData obj here and building on it in render (other than to use translate at the right time)? It seems to add some indirection for little gain.

I think we could use the property initializer shorthand to set these strings on the component like so:

static propTypes = { ... } someCopy = { copy1: this.props.translate( 'Some copy' ), copy2: this.props.translate( 'Some more copy' ), }; render() { ... }

The benefit of which being that translate would only be called once per component being created.

Yes, this definitely sounds better. Improved the code.

spen · 2018-04-09T15:23:16Z

client/my-sites/site-settings/section-import.jsx

@@ -153,8 +155,13 @@ class SiteSettingsImport extends Component {
 								<MediumImporter { ...{ key, site, importerStatus } } />
 							) ) }

+						{ isEnabled( 'manage/import/site-importer' ) &&
+							siteImporterImports.map( ( importerStatus, key ) => (


Here, and inn other mappings I see the index used as a key - Is this not something we should be concerned about? I know that generally folk see it as an anti-pattern (# # and the official react docs mention that index should only be used as a last resort. I'm not so sure if there is anything better in the data though so please ignore if the index is the only differentiation we can make here :)

Yes, this is part of the legacy code :) We'll iterate and refactor the section to follow more recent practices and make it work/look better.

spen · 2018-04-09T15:24:56Z

client/my-sites/importer/site-importer/site-importer.jsx

+		const { title, icon, description, uploadDescription } = this.props.importerData;
+		const site = this.props.site;
+		const state = this.props.importerStatus,
+			isEnabled = appStates.DISABLED !== state.importerState,


nit: should we use const for each line here?

spen · 2018-04-09T15:25:16Z

client/my-sites/importer/site-importer/site-importer.jsx

+ * Module variables
+ */
+const compactStates = [ appStates.DISABLED, appStates.INACTIVE ],
+	importingStates = [


nit: should we use const for each line here?

spen · 2018-04-09T15:28:13Z

client/my-sites/importer/site-importer/site-importer-input-pane.jsx

+}
+
+const mapDispatchToProps = dispatch => ( {
+	mapAuthorFor: importerId => ( source, target ) =>


I may be wrong, but would it not be worth creating a new action for this? It seems like a lot of logic for the mapDispatchToProps cb

It was the easiest way to achieve the functionality. I agree it would make more sense to extract is a separate action, but with the underlying Flux code, I don't think it makes sense to add more things there instead of focusing on migrating to Redux and improving the code here after that.

That makes sense :)

spen · 2018-04-09T15:30:02Z

client/my-sites/importer/site-importer/site-importer-input-pane.jsx

+	render() {
+		return (
+			<div className="site-importer__site-importer-pane">
+				{ this.state.importStage === 'idle' && (


I think a slight improvement here would be to make these state labels constants, so we can compare like:
this.state.importStage === IDLE_STATE && etc.

It would be awesome to extract that while refactoring the import section, so we can unify it between all the importers.

jblz

Let's get this in Dev!

Add Site Importer Beta to Calypso

da9f7d9

bisko added [Type] Enhancement [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Import labels Apr 5, 2018

bisko self-assigned this Apr 5, 2018

bisko requested review from marekhrabe and jblz April 5, 2018 14:03

jblz reviewed Apr 5, 2018

View reviewed changes

Improve code according to feedback

bbbb457

marekhrabe reviewed Apr 6, 2018

View reviewed changes

spen reviewed Apr 9, 2018

View reviewed changes

bisko added 2 commits April 12, 2018 11:57

Improve code organization

82e8c87

Improve code style

a2c6a63

bisko mentioned this pull request Apr 13, 2018

Reduxify: lib/importer #24148

Closed

jblz approved these changes Apr 13, 2018

View reviewed changes

bisko merged commit 2e3a7be into master Apr 13, 2018

bisko deleted the add/site-importer-beta branch April 13, 2018 18:41

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 13, 2018

spen mentioned this pull request Apr 18, 2018

Add site settings blogger importer #24286

Closed

talldan mentioned this pull request May 1, 2018

Migrate import store from Flux to Redux #24490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Importer: Add Site Importer Beta to Calypso #23930

Site Importer: Add Site Importer Beta to Calypso #23930

bisko commented Apr 5, 2018

matticbot commented Apr 5, 2018

jblz left a comment

jblz Apr 5, 2018

bisko Apr 5, 2018

jblz Apr 5, 2018

bisko Apr 5, 2018

jblz Apr 5, 2018

bisko Apr 5, 2018

bisko commented Apr 5, 2018

marekhrabe Apr 6, 2018

marekhrabe commented Apr 9, 2018

spen left a comment

spen Apr 5, 2018

bisko Apr 12, 2018

spen Apr 9, 2018

bisko Apr 12, 2018

spen Apr 9, 2018

bisko Apr 12, 2018

spen Apr 9, 2018

bisko Apr 12, 2018

spen Apr 9, 2018

bisko Apr 12, 2018

spen Apr 12, 2018

spen Apr 9, 2018

bisko Apr 12, 2018

jblz left a comment

Site Importer: Add Site Importer Beta to Calypso #23930

Site Importer: Add Site Importer Beta to Calypso #23930

Conversation

bisko commented Apr 5, 2018

matticbot commented Apr 5, 2018

jblz 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

bisko commented Apr 5, 2018

Choose a reason for hiding this comment

marekhrabe commented Apr 9, 2018

spen 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

jblz left a comment

Choose a reason for hiding this comment