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

Fix multiselection for float elements #11748

Merged
merged 4 commits into from
Nov 12, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 12, 2018

When we have float elements on the page, multi-selection triggers too often. The idea of this PR is to prevent multi-selection from triggering when the selected block is a float and the cursor is still between the top and the bottom of the block.

Fixes #11698.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Block Multi Selection The ability to select and manipulate multiple blocks labels Nov 12, 2018
@youknowriad youknowriad self-assigned this Nov 12, 2018
@youknowriad youknowriad requested review from jasmussen and a team November 12, 2018 08:04
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

OMG this is amazing. Holy guacamole this is lovely. We need to clone you.

This fixes it for me.

Code seems good and small, but if you need an additional sanity check on that feel free to do so.

@jasmussen
Copy link
Contributor

This PR fixes #11698.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Code works for me! :shipit:

@youknowriad youknowriad merged commit 4c487f7 into master Nov 12, 2018
@youknowriad youknowriad deleted the fix/multi-selection-trigger-float branch November 12, 2018 11:00
@jasmussen
Copy link
Contributor

💥

@@ -78,8 +79,15 @@ class BlockList extends Component {
this.props.onStartMultiSelect();
}

const boundaries = this.nodes[ this.selectionAtStart ].getBoundingClientRect();
const y = clientY - boundaries.top;
const blockContentBoundaries = getBlockDOMNode( this.selectionAtStart ).getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason for the switch to getBlockDOMNode? Is there a hope to get rid of this this.nodes value altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that the this.nodes refer to the parent dom element which has a height = 0 for floats and I wanted the element surrounding the actual content of a block.

const y = clientY - boundaries.top;
const blockContentBoundaries = getBlockDOMNode( this.selectionAtStart ).getBoundingClientRect();

// prevent multi-selection from triggering when the selected block is a float
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything about this which is really specific to floats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I thought I'd mention because it allows people to understand the reason for this code.

youknowriad added a commit that referenced this pull request Nov 12, 2018
* Fix multiselection for float elements

* Update changelog

* Add a comment to explain the fix

* revert unrelated change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Multi Selection The ability to select and manipulate multiple blocks [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