-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
There was a problem hiding this 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.
This PR fixes #11698. |
There was a problem hiding this 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!
💥 |
@@ -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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
* Fix multiselection for float elements * Update changelog * Add a comment to explain the fix * revert unrelated change
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.