Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Editor: Enable global drag and drop of files #2447
Conversation
youknowriad
self-assigned this
Aug 17, 2017
youknowriad
requested review from karmatosed, mtias, and jasmussen
Aug 17, 2017
Why not? Ideally the global drag and drop would work like the inserter pointer—depending on where you drag it shows the inserter pointer to insert a new image block in that place. That granularity should make it possible to still drag on top of an image placeholder and have that take over. |
|
This works and is a great improvement. But I agree with Matías, it would be super awesome if when you drag between two blocks, you get that blue line indicator for where the block is going to land. |
For now, we have a |
|
Can I have some design input about which drop zone should show up when dragging a file and when? This could help me understand and implement it. |
Sure, let me mock something up. |
|
Something like this, for when you're dragging into the editor:
Something like this when you're dragging on to a placeholder:
Is this doable? |
|
@jasmussen Can you take a look at the current state? |
|
We're getting closer!
I'm not sure it's the perfect approach, though — you still have to hit the little dropzones, where it'd be nicer if, as soon as you move the file over the editor, the blue line appears somewhere, to indicate where it'll land if you let go of it. Right now if you let go anywhere but over the little dropzones, it doesn't upload. Medium does this pretty well:
I realize this is probably endlessly difficult. Thank you for working on it. |
|
@jasmussen I guess it's possible to do what medium does only if we disable the drop zone of the image block. Because if we leave it, we'd have two drop zones overlapping each other. |
|
@jasmussen Thinking about this, maybe we can achieve something close to this. It'll take time but I'll try something. |
|
@jasmussen It should be better now :) |
It is! Wow you work fast! In chrome on the demo content I'm experiencing some slowness, though. It feels as if only after I release the mouse, does the blue line appear, and it feels like there's some runaway process because the browser gets slow. Tried to record this gif:
It seems like the behavior is right, though! |
|
@jasmussen I don't experience the slowness myself but I think I know where this comes from, the last update may help. |
|
I still really like this PR. The slowness seems better, but it's still pronounced on the demo content. Here are two gifs — this is with a new post:
In this one it feels pretty good. It would be nice if the blue line was the same color and thickness of the line you get from the inserter. It would also be nice if, when dropping in, you get like a gray placeholder with a spinner on it, to imply that it's now added and being uploaded. This is fine to revisit in a separate PR, though. In the demo content, it's slower, though:
I don't know what to do here — it seems like the more content you add to a post or page, the slower it gets. |
Yes, There's an event I'm using "dragover" that is being triggered too many times, I tried "debouncing" the callbacks but I guess it's not enough. I will keep looking how to improve this. |
|
I did some micro-optimizations. I don't know if you feel it's enough. If it's not the case, there is an alternative but a code a bit more complex. Let me know, how's the laginess. |
|
I hate to say it, but it doesn't feel super much better yet on the demo content :( |
|
@jasmussen Thanks for testing. I'm trying the alternative now. |
|
I split the dropzone logic now between two components A provider and the dropzone itself. This allows us to avoid multiple global events handler and solves the performance issues. @aduth any thoughts? |
|
Wanted to just add that yes, perf is |
aduth
reviewed
Aug 22, 2017
Should we / how should we support dropping an image into a new post, since the current implementation assumes it's being dropped before or after an existing block?
Can we create a block immediately upon dropping the image? Especially noticeable for large images which can take some time to upload, I thought it wasn't working.
| + this.dropzones.push( { element, updateState, onDrop, onFilesDrop } ); | ||
| + }, | ||
| + remove: ( element ) => { | ||
| + this.dropzones = filter( this.dropzones, ( dropzone ) => dropzone.element !== element ); |
aduth
Aug 22, 2017
Member
I wish Lodash had a _.withoutBy so we could stop iterating once the omitted match is found.
| + // DropZone provider: | ||
| + [ | ||
| + DropZoneProvider, | ||
| + {}, |
youknowriad
Aug 22, 2017
Collaborator
Pretty sure this was causing a warning or an error. Will try again.
| @@ -337,6 +357,9 @@ class VisualEditorBlock extends Component { | ||
| aria-label={ blockLabel } | ||
| { ...wrapperProps } | ||
| > | ||
| + <DropZone |
aduth
Aug 22, 2017
Member
Does this need to be in VisualEditorBlock? Would it be worth creating a separate wrapper component containing the dropFiles behavior to render here instead?
Just trying to think of ways to de-bloat this file.
| @@ -383,7 +406,7 @@ class VisualEditorBlock extends Component { | ||
| focus={ focus } | ||
| attributes={ block.attributes } | ||
| setAttributes={ this.setAttributes } | ||
| - insertBlocksAfter={ onInsertBlocksAfter } | ||
| + insertBlocksAfter={ ( blocks ) => onInsertBlocks( blocks, order + 1 ) } |
aduth
Aug 22, 2017
Member
This seems like a component we might want to make pure, in which case we'd want to be careful about creating new function references on every render. Maybe this should point to a pre-bound instance method?
(Then again, we don't yet expose or take advantage of PureComponent, so this advice is premature)
| + media.id = savedMedia.id; | ||
| + media.url = savedMedia.source_url; | ||
| + if ( gallery ) { | ||
| + setAttributes( { images: [ |
aduth
Aug 22, 2017
Member
I know this existed previously, but it seems odd to me that we need to handle the setAttributes logic here, especially because it forces us to fork the logic depending on the block type. Shouldn't this function just return the promise that resolves as the savedMedia object, encapsulating only the steps necessary to create and send the media payload?
youknowriad
Aug 22, 2017
Collaborator
Yep, I thought the same thing when I saw it. I guess it's like that because it's has been extracted from the component. But the Promise is not enough (unless we have a notify API) to show the "loading" state.
youknowriad
Aug 22, 2017
Collaborator
I'm leaving this and the image placeholder while uploading to a follow-up PR. They are related and they require some thoughts on how to update block attributes from a transformation after the creation of the block.
| @@ -119,5 +119,6 @@ export default connect( | ||
| onRedo: redo, | ||
| onUndo: undo, | ||
| onRemove: removeBlocks, | ||
| + insertBlocks, |
Responding from a non technical standpoint: yes! Whether it's an image placeholder, a preview of the image (cloudup does this), or something else, this could potentially address the issue of nothing happening visually between the time where you drop the image and the image appears. Not that I think it has to hold up the pr, but wanted to chime in. |
|
Extracted the BlockDropZone to its own component and I was able to reuse it in the empty post placeholder. |
codecov
bot
commented
Aug 22, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #2447 +/- ##
==========================================
- Coverage 28.99% 28.82% -0.18%
==========================================
Files 167 169 +2
Lines 5063 5100 +37
Branches 836 844 +8
==========================================
+ Hits 1468 1470 +2
- Misses 3055 3082 +27
- Partials 540 548 +8
Continue to review full report at Codecov.
|
| + | ||
| +function BlockDropZone( { index, ...props } ) { | ||
| + const dropFiles = ( files, position ) => { | ||
| + const transformation = reduce( getBlockTypes(), ( ret, blockType ) => { |
aduth
Aug 22, 2017
Member
Should this be find instead of reduce? Why continue iterating the list if we only want first result?
const transformation = find( getBlockTypes(), ( ret, blockType ) => {
return find( get( blockType, 'transforms.from', [] ), ( transform ) => (
transform.type === 'files' && transform.isMatch( files )
) );
} );
youknowriad
Aug 22, 2017
Collaborator
This won't work because I'm returning the transformation and not the blockType, the return value of find callback is considered a boolean right?
aduth
Aug 22, 2017
•
Member
Ah, that's true. Alternatively it could be a find on the reduced block transforms, but not sure this really saves us much aside from some compactness, since it still needs to iterate over all block types.
const transformation = find(
getBlockTypes().reduce( ( result, blockType ) => (
result.concat( get( blockType, 'transforms.from', [] )
), [] ),
( transform ) => transform.type === 'files' && transform.isMatch( files )
} );I don't feel strongly either way.
| + if ( index !== undefined ) { | ||
| + insertPosition = position.y === 'top' ? index : index + 1; | ||
| + } | ||
| + transformation.transform( files ).then( ( blocks ) => { |
aduth
Aug 22, 2017
Member
Do we want to assume transform would always return a promise? Are there not synchronous cases? Any concern about disjointedness between other transforms that don't support promises?
youknowriad
Aug 23, 2017
Collaborator
What alternative could you suggest in the case of file transformations?
aduth
Aug 23, 2017
Member
I'm not saying we need an alternative necessarily, just to consider:
- Could it ever be synchronous? Do we want to support this?
- Impact of consistency between other transform return values
youknowriad
Aug 23, 2017
Collaborator
Should we drive this by example? For now, we only need to support asynchronous file transformation and synchronous transformation for the other transforms. Thinking we should keep it like this unless we discover a valid use-case.
youknowriad
added some commits
Aug 17, 2017
aduth
approved these changes
Aug 23, 2017
Few small behavior quirks which I think could be fixed up in subsequent pull requests:
- The droppable area can be very particular. I noticed especially trying to drop an image before the first paragraph in a post.
- When dropping an image on a new post, the "Drop files to upload" labeling is pretty tight (hard to take a screenshot of this behavior, sorry)
- Should have immediate reaction to image being dropped, not waiting for upload to finish
youknowriad commentedAug 17, 2017
tries to address #2327
If we go this road, we won't be able to have both "global drag and drop" and "drag and drop in specific blocks".
For now, I implemented this for the image block. If you drag an image, it will create an Image Block.
I can do the same for The Gallery and the video blocks but wanted to ask for review first before continuing.