Skip to content
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

Disable drag on images to enable multi-select #1014

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 3, 2017

This may be a bit controversial, but in order to enable multi-select starting at image blocks with the current behaviour, we need to disable dragging them. Dragging in the current WP core editor is used to move the image. We now have the block movers for this, and it make be better to find a solution for drag-and-drop for all blocks that is consistent rather than images alone. (Also note that dragging the images will not work to move them at the moment, it would have to be implemented. You can drag it in an Editable region though...)

@ellatrix ellatrix self-assigned this Jun 3, 2017
@jasmussen
Copy link
Contributor

I think it's probably for the best to disable dragging regardless, at least for a block such as the image block. It's something that we could consider enabling for the freeform block.

Selecting starting at the image works as intended, nice!

I'm seeing the focus style on the trash button permanently:

screen shot 2017-06-03 at 15 04 34

Is the blue selection header not in master yet? Not seeing it here.

The flickering when selecting across blocks, I can't tell if it got worse in this PR or not. But it's definitely something we'll want to look at at some point.

Otherwise, works well!

@ellatrix
Copy link
Member Author

ellatrix commented Jun 3, 2017

I almost forgot: I should note that this disables any kind of dragging from inside the block, also text dragging. Notice that in master you can select some text, and then drag the selected text to another Editable region. Not sure if we should disable that. Maybe we should do some filtering based on the target.

@ellatrix
Copy link
Member Author

ellatrix commented Jun 3, 2017

The header thing is still in #955. Yeah, I should look at the flickering next.

@mtias
Copy link
Member

mtias commented Jun 5, 2017

I like this behaviour, it's more consistent with what we are building, and make our core interactions more reliable.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do some filtering based on the target.

Prevent by nodeType === Node.ELEMENT_NODE ?

@ellatrix
Copy link
Member Author

ellatrix commented Jun 5, 2017

Interesting. event.nativeEvent.target is not the same as event.target. React seems to give the parent node for text nodes. Checking if the target is a text node may be reliable. I'd think the data would be at least plain text, but the browser even wraps it in a span with styles (drag quote inside citation). This will probably fixed together with cleaning pasted content.

@ellatrix
Copy link
Member Author

ellatrix commented Jun 5, 2017

(If React adjusts the target, it may mean that it's not consistent across browsers. Looking.)

@ellatrix
Copy link
Member Author

ellatrix commented Jun 5, 2017

Let's ignore all for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants