Editor: Enable Drag/Drop for Featured Image component in the editor #11839

bisko · 2017-03-07T16:07:09Z

This PR enables Drag/Drop functionality over the Featured Image component in the sidebar of the editor as mentioned in #324.

The scope of this PR is just to add the DropZone to the sidebar accordion and allow to upload a featured image through there. Future improvements to the feature and adding additional dropzones will be targets of other, separate, PRs.

How it looks as of f64fa15:

previous versions: #

To test:

Checkout branch or use Calypso.live link
Start editing a post or create a new post
Drag an image over the featured image block in the sidebar
Verify the dropzone shows
Verify that the dropped image is set as the featured image
Verify that saving the post would retain the featured image
Verify that there are no JS errors in the console
Verify that you can still upload a featured image when clicking the button (old way)
Verify you can upload image on a mobile device (touch)

matticbot · 2017-03-07T16:07:10Z

Test live: https://calypso.live/?branch=add/components-editor-featured-image-drag-drop

lsinger · 2017-03-07T17:26:21Z

/cc @shaunandrews — I think we'd like your input here.

lsinger · 2017-03-07T17:27:10Z

GIF of current state:

mcsf · 2017-03-07T17:30:01Z

client/post-editor/editor-drawer/featured-image.jsx

@@ -32,25 +36,50 @@ class EditorDrawerFeaturedImage extends Component {
 		stats.recordEvent( 'Featured image removed' );
 	}

+	onFilesDrop = ( files ) => {
+		Dispatcher.register( function( payload ) {


I wouldn't have expected to have to extend the old dispatcher. Questions:

why is it needed?

if it is, can we extend a preëxisting Dispatcher.register callback?

if it is, could this be better accomplished with one-off subscriptions (e.g. eventEmitter.once( cb ))?

if it is, could this be better accomplished by relaying these into Redux?

I'm assuming it's a temporary thing, but right now this obviously will add a new handler to Dispatcher on each filesdrop event.

From bottom to top:

I'm assuming it's a temporary thing, but right now this obviously will add a new handler to Dispatcher on each filesdrop event.

Yes, this is temporary, just to get the functionality working.

why is it needed?

if it is, could this be better accomplished by relaying these into Redux?

MediaActions is currently sending events for the upload state through Flux. I want to bring that to Redux if possible.

if it is, can we extend a preëxisting Dispatcher.register callback?

if it is, could this be better accomplished with one-off subscriptions (e.g. eventEmitter.once( cb ))?

If we manage to push the state updates to Redux, these won't be needed at all.

mcsf · 2017-03-07T17:30:26Z

client/post-editor/editor-drawer/featured-image.jsx

+		} );
+
+		MediaActions.clearValidationErrors( this.props.site.ID );
+		MediaActions.add( this.props.site.ID, files ).then();


.then( crickets ); 😄

I will have to dig a bit further in WebAudio for that request 😋
It's a leftover from researching how MediaActions behaved for adding media to a site.

bisko · 2017-03-08T15:22:33Z

Now the DropZone is limited to the content of the Accordion and will not activate the drop events if the file is dropped over the Accordion title.

bisko · 2017-03-17T16:02:17Z

Reduxification of the feature is possible, but would require improvements in the FeaturedImage component too.

davewhitley · 2017-03-17T16:17:33Z

Basing this feedback on the gif in the original post.

It looked really strange to see both drop zones activate when you drag the image over the viewport, but now I realize that makes sense.

The icon and text look much too large in the featured image drop zone. I think more concise text could be used as well.

marekhrabe · 2017-04-12T11:29:44Z

client/components/drop-zone/index.jsx

@@ -25,11 +25,12 @@ import {

 export const DropZone = React.createClass( {
 	propTypes: {
+		customClass: PropTypes.string,


Just for more consistency, I would be all for using the general className here instead of customClass (not to be found anywhere else in Calypso). We do it in other places where we want to pass a custom class that the component will add to their internal styling (Card component just to name one).

Great suggestion. Turns out one should not be afraid of the linter rules for class name prefixes :)

Updated to className

marekhrabe · 2017-04-12T11:36:17Z

client/post-editor/editor-featured-image/dropzone.jsx

+		const postId = getEditorPostId( state );
+
+		return { siteId, postId };
+	},


As we are not doing anything here with those values, it might be worth simplifying to just:

( state ) => ( { siteId: getSelectedSiteId( state ), postId: getEditorPostId( state ), } ),

marekhrabe · 2017-04-12T11:37:26Z

client/post-editor/editor-featured-image/dropzone.jsx

+			<DropZone
+				customClass="drop-zone__custom-featured-image"
+				icon={ <FeaturedImageDropZoneIcon /> }
+				textLabel={ this.props.translate( 'Set as Featured Image' ) }


Would you update to this recommended string or should we use a new one for this case?

marekhrabe · 2017-04-12T11:47:17Z

I also noticed that at this iteration, the blue (content) dropzone might be little confusing because of its general label "drag & drop image files…" and being rendered on top of the featured image. I know this will be solved in near future by adding another dropzone into the content area, but in the meantime, we might make this more clear by providing a more exact label to the blue dropzone ("drag & drop to add to content" or something like that). What do you think? We were planning to introduce the new string for that soon anyway.

a8ci18n · 2017-04-12T16:26:00Z

client/post-editor/editor-featured-image/dropzone.jsx

+			<DropZone
+				className="editor-featured-image__dropzone"
+				icon={ <FeaturedImageDropZoneIcon /> }
+				textLabel={ this.props.translate( 'Set as Featured Image' ) }


Hi! I've found a possible matching string that has already been translated 89 times:
translate( 'Set featured image' ) ES Score: 9

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

mcsf

Looks great! Pending some of my questions and suggestions, this seems GTM.

mcsf · 2017-04-06T17:31:32Z

client/post-editor/editor-drawer/featured-image.jsx


 		return (
-			<Accordion title={ translate( 'Featured Image' ) } initialExpanded={true}>
+			<Accordion title={ translate( 'Featured Image' ) } initialExpanded={ true }>


Pro-tip: can be <Accordion title={ … } initialExpanded>

mcsf · 2017-04-12T16:08:04Z

client/components/drop-zone/index.jsx

@@ -251,7 +253,8 @@ export const DropZone = React.createClass( {
 			'is-active': this.state.isDraggingOverDocument || this.state.isDraggingOverElement,
 			'is-dragging-over-document': this.state.isDraggingOverDocument,
 			'is-dragging-over-element': this.state.isDraggingOverElement,
-			'is-full-screen': this.props.fullScreen
+			'is-full-screen': this.props.fullScreen,
+			[ this.props.customClass ]: true,


Might as well place this as a direct argument of classNames, right after 'drop-zone'

I was trying to figure that out, but I couldn't quite figure out the classNames documentation on that :) Wanted to make sure it works. Thanks for pointing me to the right way!

mcsf · 2017-04-12T16:08:46Z

client/components/drop-zone/style.scss

@@ -34,7 +40,8 @@
 	z-index: z-index( 'root', '.drop-zone__content' );
 	transform: translateY( -50% );
 	width: 100%;
-	font-size: 24px;
+	font-size: 14px;
+	font-weight:bold;


Space before bold :P

mcsf · 2017-04-12T16:09:40Z

client/components/drop-zone/style.scss

+	}
+}
+
+.is-hidden {


Should we be more specific than .is-hidden? I don't know enough to answer.

Thanks for pointing me to that. The CSS was in the wrong place :)

The question about the class name:

The class is set when the DropZone is being rendered on the screen. It hides the editor-drawer-well content that is in the Featured Image block, because otherwise it shows up behind the dropzone and doesn't look good.

Comparsion:

It could use better naming, because is-hidden is very general in meaning, but I don't feel comfortable to naming it is-dropzone-visible because it may be used in some other case in the future.

mcsf · 2017-04-12T16:16:14Z

client/post-editor/editor-drawer-well/index.jsx

+				{ this.props.customDropZone
+					? this.props.customDropZone
+					: null
+				}


I wonder: did { this.props.customDropZone } not work?

Turns out it did work!

mcsf · 2017-04-12T16:17:35Z

client/post-editor/editor-drawer/featured-image.jsx

+
+		this.state = {
+			isSelecting: false
+		};


state is already being init'd just above (state = {}), so it seems right to move isSelecting: false into that, and ditch constructor altogether.

Good catch! Updated :)

mcsf · 2017-04-12T16:21:34Z

client/post-editor/editor-featured-image/dropzone.jsx

+class FeaturedImageDropZone extends Component {
+	static contextTypes = {
+		store: PropTypes.object
+	};


We can kill this now. 🔪

mcsf · 2017-04-12T16:26:54Z

client/post-editor/editor-featured-image/dropzone.jsx

+			<DropZone
+				customClass="drop-zone__custom-featured-image"
+				icon={ <FeaturedImageDropZoneIcon /> }
+				textLabel={ this.props.translate( 'Set as Featured Image' ) }


I think Set as Featured Image is better for this particular case.

mcsf · 2017-04-12T16:28:09Z

client/post-editor/editor-featured-image/index.jsx

@@ -120,7 +121,11 @@ class EditorFeaturedImage extends Component {

 		return (
 			<div className={ classes }>
-				{ site && featuredImageId && <QueryMedia siteId={ site.ID } mediaId={ featuredImageId } /> }
+				{
+					site && featuredImageId && isNumber( featuredImageId )


Ooh, type checks. :) What is this preventing, btw?

The QueryMedia component expects a numeric image ID when querying for media. While the featured image is uploaded, a temporary transient image is used and its ID is in the form featured-image<n>, which is not numeric.

Full error:

Warning: Failed prop type: Invalid prop `mediaId` of type `string` supplied to `QueryMedia`, expected `number`.

Also while the transient is used, the image doesn't actually exist yet on the server side, which means an extra failed request is done for it.

The isNumber check makes sure the image is created on the backend, before it's queried by QueryMedia

💯 would ask for explanation again

marekhrabe · 2017-04-13T14:56:45Z

Thanks for addressing a ton of things and making this ready! I'm not seeing anything that should prevent us from merging now :)

davewhitley · 2017-04-13T17:04:16Z

Adding a design review, since the label is still up. I think the new drop zone style is fine, looks pretty good to me. I find it strange that the featured image text is black and the regular blue zone has white text. Still think we should just use a generic Gridicon for featured image, since the star overlaying the image icon partially creates a new icon that is inconsistent with the Gridicons style. All can be addressed later though, since this just got merged.

****** TODO: refine this message before merging ***** html mode dropzone introduced in #11680. at the time, there weren't any other dropzone -- the sidebar featured image zone was added a week later, in #11839 -- so the html mode dropzone took up the entire screen. because it could take up the entire screen, its placement inside the editor-html-toolbar didn't pose any tangible problems. when a third dropzone was added in #16819, this dropzone was made not-full-screen so that the new dropzone would be accessible. by accident, though, the changes in this PR were not done at that time, which led to the media dropzone being only 10px tall, instead of covering the entire editor body because there are now multiple dropzones on the editor screen, the html mode dropzone should only cover the editor body itself, not the entire screen. in order to achieve that, it needs to be moved above the editor-html-toolbar, since the toolbar is restricted to 40px tall.

When in HTML-mode, the Editor has a dropzone for uploading files and inserting them into the post body. Because of #18535, that dropzone is currently only 10px tall, when it should take up the entire body of the editor. The editor's HTML-mode media dropzone was introduced in #11680. At the time, there weren't any other dropzones on the screen -- the sidebar featured image zone was added a week later, in #11839 -- so the HTML-mode dropzone took up the entire screen. Because it could take up the entire screen, its placement inside the editor-html-toolbar didn't pose any tangible problems. When a third dropzone was added in #16819, the HTML-mode dropzone has its fullscreen prop disabled, so that the new dropzone would be accessible. The changes in this PR should have been done at that time, but it was overlooked. Because they weren't done, the media dropzone is currently only 10px tall, instead of covering the entire editor body. This makes it difficult for someone to drop a file in the correct location. Because there are now multiple dropzones on the editor screen, the HTML-mode dropzone should only cover the editor body itself, not the entire screen. In order to achieve that, it needs to be moved above the editor-html-toolbar in the DOM, since the toolbar is restricted to be 40px tall. Fixes #18535

bisko added Components [Feature] Post/Page Editor The editor for editing posts and pages. [Status] In Progress [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Mar 7, 2017

bisko self-assigned this Mar 7, 2017

bisko requested a review from lsinger March 7, 2017 16:07

bisko requested review from lancewillett, mcsf, marekhrabe and michaeldcain March 7, 2017 16:07

matticbot added the [Size] M Medium sized issue label Mar 7, 2017

bisko changed the title ~~Editor: Enable DropZone drag/drop for Featured Image component in the editor~~ Editor: Enable Drag/Drop for Featured Image component in the editor Mar 7, 2017

mcsf reviewed Mar 7, 2017

View reviewed changes

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Mar 8, 2017

bisko force-pushed the add/components-editor-featured-image-drag-drop branch from 04dfdba to 36be5c3 Compare March 13, 2017 14:48

matticbot added [Size] L Large sized issue [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Mar 13, 2017

bisko mentioned this pull request Mar 16, 2017

Components: DropZone accepts custom text label #12234

Merged

bisko force-pushed the add/components-editor-featured-image-drag-drop branch from 2f8e63a to 686a0a3 Compare March 17, 2017 15:08

bisko added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels Mar 17, 2017

marekhrabe reviewed Apr 12, 2017

View reviewed changes

Change customClass property to className

ad57370

a8ci18n reviewed Apr 12, 2017

View reviewed changes

mcsf approved these changes Apr 12, 2017

View reviewed changes

bisko added 8 commits April 12, 2017 19:34

Improve code style

9322b52

Improve code style

12f3257

Improve classNames parameters

e27c26a

Improve whitespace in css

0fa10cd

Remove unnecessary typechecking

b9817a5

remove extra contextTypes

0e19fd0

Add analytics for featured image uploads

8fd8e02

Move CSS code to more relevant place

be34c95

marekhrabe added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 13, 2017

michaeldcain approved these changes Apr 13, 2017

View reviewed changes

bisko merged commit 5abaca2 into master Apr 13, 2017

bisko deleted the add/components-editor-featured-image-drag-drop branch April 13, 2017 17:00

matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Ready to Merge labels Apr 13, 2017

This was referenced Apr 20, 2017

Editor: Auto-expand Featured Image meta box with drag and drop #13275

Closed

Editor: Add featured image drop zone above title #13277

Closed

bisko mentioned this pull request Apr 20, 2017

Editor: Automatically expand the Featured Image accordion when an image is dragged on the page #13283

Merged

2 tasks

lsinger mentioned this pull request Apr 24, 2017

Editor: Allow dropping on featured image #324

Closed

marekhrabe mentioned this pull request Jun 7, 2017

Editor: Feature Image Dropzone Visual Feedback #14847

Closed

iandunn mentioned this pull request Oct 5, 2017

Editor: Move HTML-mode media dropzone higher in DOM to cover editor body #18558

Closed

Editor: Enable Drag/Drop for Featured Image component in the editor #11839

Editor: Enable Drag/Drop for Featured Image component in the editor #11839

Conversation

bisko commented Mar 7, 2017 • edited Loading

matticbot commented Mar 7, 2017

lsinger commented Mar 7, 2017

lsinger commented Mar 7, 2017

Choose a reason for hiding this comment

bisko Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bisko commented Mar 8, 2017

bisko commented Mar 17, 2017

davewhitley commented Mar 17, 2017

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

marekhrabe commented Apr 12, 2017

Choose a reason for hiding this comment

mcsf 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

The question about the class name:

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

marekhrabe commented Apr 13, 2017

davewhitley commented Apr 13, 2017 • edited Loading

bisko commented Mar 7, 2017 •

edited

Loading

bisko Mar 8, 2017 •

edited

Loading

davewhitley commented Apr 13, 2017 •

edited

Loading