Add hovered block store and show hovered template part's label in edit site header component. #25348
Conversation
|
Size Change: +303 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Pausing this here briefly to implement a rough idea of another approach so we can compare before moving forward. |
ZebulanStanphill
added
the
Needs Accessibility Feedback
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
Addison-Stavlo
changed the title
Addison-Stavlo
removed
the
Needs Accessibility Feedback
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
|
Instead of limiting by timestamps, a better option could be to limit on mouse position of the last evaluation. While simultaneous events may have slightly different timestamps in some browsers, it seems the mouse positions are always the same. However, we would need to account for scrolling. Otherwise, when the mouse is in the same position and we scroll down the document it would never re-evaluate what is hovered.
We could:
Number 2 would be good if there is already a way to do this. 🤔 Kind of a shame pageX/pageY won't work on their own. |
To clarify, I'm not the most familiar with scroll attributes, but couldn't we evaluate To be more specific, I tested this in the chrome dev console. |
Yeah we could do that. I was hoping there was a better way to get ahold of that info without |
Tried this for now. Although it would be nice to get rid of that timestamp, we will need some way to detect scroll position in combination of mouseCoords for that. The timestamp is now just used to allow scrolling over blocks with the cursor in the same position to re-trigger the evaluation. |
| // event firing from multiple nested blocks. Checking the time allows us to re-evaluate | ||
| // even when mouseCoords have not changed, such as when scrolling via mouse-wheel. | ||
| // However, this deltaTime checked must be sufficiently long to ensure slow CPUs do not | ||
| // trigger unnecessary recalculations for events triggered simultaneously. | ||
| const { time, mouseCoords } = |
There was a problem hiding this comment.
Scroll location would be much better to check than the last timeStamp to ensure this works on scrolling as well. However, the scroll location corresponds to the interface-interface-skeleteton__content which is contained in the interface package. Whether we found a way to reference this element or querySelector'd for it here, we would make this functionality dependent on a separate package which is also not ideal. The time check will work, but fast mouse-wheel scrolls may not always re-trigger the evaluation (not the end of the world 🤷♀️ , I figure it is better to try to be safer with performance).
|
Im thinking this should be more or less 'ready to go' at this point. |
There was a problem hiding this comment.
I'm in favor of merging this to try it out. The code looks good, and it tests well locally. Thanks for all the iterations @Addison-Stavlo!
I don't think we should spend any more time optimizing it though. So if folks don't agree that we should merge and try out, I think we should move on from this for now.
There was a problem hiding this comment.
I'm also in favor of trying this out, we can always revise the approach later if we come up with a better way to resolve the underlying issue. It tests well for me and the code is looking good. Nice work @Addison-Stavlo! ✨
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
| eventCoords | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if there might be something in common with the multi-selection hook where we do similar things. cc @ellatrix
| const { hoveredBlockIds, timeStamp, mouseCoords } = action; | ||
| return { | ||
| hoveredBlockIds, | ||
| eventData: { |
There was a problem hiding this comment.
Why do we need thisi information in state?
There was a problem hiding this comment.
the scenario is: "move mouse over a block which is deeply nested". This will trigger N hover events for N nested blocks. If we have a ton of those blocks, we'll be dispatching to the store N times, which caused performance issues. To avoid updating the store that often, we need to compare the timestamp of an event with the timestamp of another event. So we track the timestamp here. I think the other option would be storing it in a global variable in the file where it's used
There was a problem hiding this comment.
Is there a way to "stop propagation" somehow on the event?
There was a problem hiding this comment.
i'm unsure, I'll leave that for @Addison-Stavlo to answer
There was a problem hiding this comment.
Is there a way to "stop propagation" somehow on the event?
I didn't have any luck. It's not one event bubbling, but many events being triggered simultaneously.
I think we can get around requiring this information and extra complexity by debouncing. e41e82d
There was a problem hiding this comment.
That said, while having a way to track which blocks are hovered is neat, given the comments from design below it doesn't seem like we have a consensus on where we should use this information yet (probably not in the document actions header from the sounds of it 😆 ). This could prove useful in implementing something similar in the future (such as closer to the cursor #25348 (comment)), but until we figure out the right place to utilize this there is no rush for adding the handlers and store either.
|
I have mixed feelings here. One side of me likes the interactivity and the ability to quickly see what template parts make up my document; But the other side of me things maybe there's too much changing as I move my mouse pointer around the screen. As is, the transition doesn't handle nested template parts well; I'd expect the label to animate even for nested template parts. -- I think for the normal edit mode, this might be too much motion on the screen. However, maybe it could make sense as part of select mode, which already shows block borders on hover. |
|
I'd agree that the visual feedback can be helpful, but if we want to add something I'd like for it to be closer to the cursor. Trying to mouse over my Footer template part, but having to look at the top of my screen to understand when I've hit the target is kind of uncomfortable. |
|
a bit old and stale, past due for a close button click. |
Description
Adds a way to track hovered blocks by adding them as an array of clientIds in the
block-editorstore. This information is then used to show the hovered template part's label in the component added in #25320 (alternative approaches discussed in #25469).The first approach to implement this store tried to add/remove blocks individually by clientId. However, this ran into a blocker with the inserter triggering removal for all blocks that it appeared to be within the bounds of. Parsing the blocks found under the cursor and setting the full list solves this problem, and the hovered name is no longer disrupted by the inserter as shown in the gif above.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: