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

Blocks: Fix clearing selection on canvas click #1079

Merged
merged 2 commits into from
Jun 9, 2017

Conversation

youknowriad
Copy link
Contributor

#812 added a padding around the BlockList container, so in order to dismiss the selection properly, we need to catch clicks on this container.

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Jun 8, 2017
@youknowriad youknowriad self-assigned this Jun 8, 2017
@youknowriad
Copy link
Contributor Author

I've noticed that this breaks multi-selection. Looking

@youknowriad
Copy link
Contributor Author

It's better now, I've updated the dismiss event. It's bound to onMouseUp instead. Let me know how that feels for you.

@aduth
Copy link
Member

aduth commented Jun 8, 2017

Instead of capturing the click, can we style it so that it's not occupying the full width of the canvas, letting clicks be captured by the <VisualEditor /> component?

max-width: $visual-editor-max-width;
margin: 0 auto;

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 8, 2017

@aduth I thought that was necessary for #812 but I may be wrong. Looking

Edit: Yep, I think it's not an option, this breaks the image alignment.

@jasmussen
Copy link
Contributor

jasmussen commented Jun 8, 2017

Yep, the new full width method requires there to be no max width on the blocks. We've been through a few techniques and this was the one that had the fewest tradeoffs.

User experience wise, this works well! Mousedown would be better, but absent that this works well! 👍

@aduth
Copy link
Member

aduth commented Jun 8, 2017

It's bound to onMouseUp instead.

Why is this necessary? Feels like we're letting desired behavior of this component be dictated by its descendants.

@aduth
Copy link
Member

aduth commented Jun 8, 2017

Maybe we revert to the behavior where block.js calls stopPropagation in its onClick handler, and anything else that bubbles up to VisualEditor is assumed cause the clear?

@youknowriad
Copy link
Contributor Author

Why is this necessary?

It's necessary because when we click the mouse on a node and release it on a different one, The event target is the first common ancestor I think. This causes the multi-selection to clear itself on release.

I'm updating to use onMouseDown which is better I think (as suggested by @jasmussen)

@youknowriad youknowriad force-pushed the fix/click-canvas-drop-selection branch from 1925c43 to 7606822 Compare June 8, 2017 14:23
@youknowriad
Copy link
Contributor Author

I'm merging this since it's the best solution we have right now.

@youknowriad youknowriad merged commit 845dd39 into master Jun 9, 2017
@youknowriad youknowriad deleted the fix/click-canvas-drop-selection branch June 9, 2017 11:08
@mtias
Copy link
Member

mtias commented Jun 9, 2017

Perhaps going back to clickOutside and trying to prevent it from happening if you click on the inserter or the sidebar could be worth looking back into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants