Insertion point bar: hide onBlur and onMouseLeave#36798
Merged
Conversation
|
Size Change: +12 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
74bd330 to
240d5de
Compare
Contributor
|
My goodness, even as is, this feels like a VAST and profound improvement over what's in trunk. As a reminder, this is how easy it is to stick the sibling inserter in trunk at the moment: Here's this branch:
This feels like a massive forward step. I'd love for this to land as soon as possible! Thank you! |
jasmussen
approved these changes
Nov 26, 2021
Contributor
jasmussen
left a comment
There was a problem hiding this comment.
Gave it another spin, and in all my testing I saw no downsides to this one.
The code looks minimal and good. I'd encourage just a quick sanity check from another dev, but honestly this feels good enough to land.
ntsekouras
reviewed
Nov 26, 2021
| onFocusOutside={ maybeHideInserterPoint } | ||
| onMouseLeave={ maybeHideInserterPoint } | ||
| > | ||
| <motion.div |
Contributor
There was a problem hiding this comment.
I've started looking at this and I'm not familiar with this code, but I was wondering if you have search for a solution from motion maybe something like onHoverEnd={ maybeHideInserterPoint } in this motion.div?
Member
Author
There was a problem hiding this comment.
I was wondering if you have search for a solution from motion maybe something like onHoverEnd={ maybeHideInserterPoint } in this motion.div?
Thanks for looking!
I tried it out and I don't see any difference in behaviour, so I don't see any implementation issues with it.
Was there a specific reason why we should prefer motion's eventhandler?
Member
Author
There was a problem hiding this comment.
Was there a specific reason why we should prefer motion's eventhandler?
Oh, I did find one advantage, aside from the fact that Popover doesn't specifically define an onMouseLeave event: we can perform an equality check on the even target and the current ref to make sure the event is occurring on the motion div.
I've pushed that change since I couldn't detect any noticeable divergence in behaviour.
ntsekouras
reviewed
Nov 26, 2021
Contributor
ntsekouras
left a comment
There was a problem hiding this comment.
Thanks for working on this Ramon! It would be great if @ellatrix shared her thoughts here as she has worked a lot with these parts. Should this be better handled here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/block-list/use-in-between-inserter.js#L15?
Member
Author
|
Thanks for testing, everyone!
Yes, I'm also a newbie in this area of the code so would be interested to hear if there's a better way. I couldn't see how we could attach event to the inserter in that hook. |
…over, and also the mouse leaves the horizontal line. Only hiding when the block inserter panel is not open
There seems to be no noticeable difference in behaviour, only that the Popover component doesn't specifically define an `onMouseLeave`. Also, we can more easily perform a check against the ref and event target.
240d5de to
9ebdd37
Compare
Member
Author
|
I removed the There is an edge case that this PR does not address: the inserter persists when you click the plus icon and drag your mouse away from it. This is because there's an exisiting onFocus callback on the motion div that sets |
ntsekouras
approved these changes
Nov 29, 2021
Contributor
ntsekouras
left a comment
There was a problem hiding this comment.
LGTM, thanks Ramon! Changes are minimal and make for a way better experience.
There is an edge case that this PR does not address: the inserter persists when you click the plus icon and drag your mouse away from it.
I think we can address this in a follow up, as it's already in trunk. Maybe this should be handled in useInBetweenInserter as we are only listening to mousemove event and we could add logic for detecting the dragging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Possibly, in some way, resolves #35536
Also resolves #36882
Description
This PR hides the inserter point via the
hideInsertionPointaction when the horizontal inserter bar loses focus, or when the mouse leaves.hideInsertionPointwill not fire when the block inserter panel is open.We might not have covered all cases documented in #35536, and there is no timeout, but it might help to avoid some of the concerns with the persistent inserter bar.
Try it out today!
How has this been tested?
Take a look at the various scenarios over at #35536
With the right mouse artistry you might find that the bar persists, but, at least in my testing, it doesn't persist as often with these changes.
Checklist:
*.native.jsfiles for terms that need renaming or removal).