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

Technical Feedback for showing hovered block labels in an external component. #25469

Closed
Addison-Stavlo opened this issue Sep 18, 2020 · 9 comments
Labels
Needs Technical Feedback Needs testing from a developer perspective.

Comments

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Sep 18, 2020

In working towards the designs suggested for adding Template and Template Part names in the top bar of the Site Editor (#25085), one challenge we face is determining how to communicate when a Template Part is hovered to the new component so that it may display the name of the hovered template part.

We have several ideas (and corresponding rough (but working) draft PRs) regarding how to accomplish this, but are unsure which solution may be the most appropriate for Gutenberg in general. Below, I will describe the various approaches, their pros and cons, and provide links to the rough draft PRs that give a rough idea of how these work in action. My hope in exposing these options is to receive some feedback, context, or other ideas for what may be the most appropriate solution to pursue further to enable this functionality.

A summary of the three approaches:

Solution 1 - Add listeners to all blocks and a hoveredBlocks reducer to the core/block-editor store (#25348).

Implementation:

  • Add mouse event listeners to the general block wrapper to enable tracking of every block’s hovered state. These listeners would dispatch the blocks’ clientIds to the store when moused over and remove them when the mouse leaves.
  • The new component then uses this information when the store updates to determine if a Template Part block is hovered and retrieves its label to be displayed.

Pros:

  • Adds hovered block information to the core/block-editor store that can be used for other purposes if necessary.
  • If we need to add any other blocks to the ‘list of names to show when hovered’, this can be handled completely from new components end (and potentially made extendable with a filter).

Cons:

  • Adding event listeners to every block may be unnecessary if there is not really a need for this sort of hovered block tracking elsewhere.
  • Mouse enter/leave events will be disrupted by the block inserter, creating the false impression that no Template Part is hovered when the mouse moves over an inserter area inside the Template Part. This can be seen in the draft PR as the label disappears when the internal block inserter is hovered.

Solution 2 - Add listeners to only the blocks of interest and using core/edit-site store (#25382).

Implementation:

  • Add the mouse event listeners only to the blocks of interest. For now, that is only the Template Part block (and maybe Post Content if design feels this is necessary). This block would then dispatch to add/remove its clientId from the store.
  • Set up a nested reducer in core/edit-site store for our new component (since it is only currently intended for the Site Editor).
  • The new component retrieves the label for the clientId of the hovered block.

Pros:

  • Dispatches happen less often since they only happen from our block of interest.
  • There are less hovered blocks for the component to parse through.

Cons:

  • Adding more blocks to the ‘list of names to show when hovered’ requires us adding event listeners and dispatches from those blocks themselves. Perhaps this could be made more reusable with something like a supports hook, but either way we would need to enable the blocks directly.
  • Using editor specific store dispatches within a block is generally not advised.
  • Mouse enter/leave events will be disrupted by the block inserter, creating the false impression that no Template Part is hovered when the mouse moves over an inserter area inside the Template Part. This can be seen in the draft PR as the label disappears when the internal block inserter is hovered.

Solution 3 - Track mouse movement and parse elements under the cursor (#25423).

Implementation:

  • Add a ‘mousemove’ listener to the document.
  • Use elementsFromPoint to retrieve the elements under the cursor.
  • Filter through these elements for a Template Part and retrieve the label.

Pros:

  • Using elementsFromPoint solves the issue with the interfering inserter. This sounds small, but in practice feels much better.
  • Changes can be confined to the new component itself.
  • If we need to add any other blocks to the ‘list of names to show when hovered’, this can be handled completely from new components end (and potentially made extendable with a filter).
  • Does not require any new reducers or dispatching.

Cons:

  • ‘Mousemove’ fires a lot! Although, the amount of calculation that needs to be done is minimal. A naive look into performance (WIP - try dom based hover tracking for document dropdown sublabel. #25423 (comment)) makes this seem reasonable.
  • DOM manipulation from React can be seen as pretty ‘hacky’ and volatile. Although a key point is we are not manipulating anything from the DOM, only parsing what is under the cursor.

I do like how approach 3 behaves with the inserter, as well as its simplicity. However, I am still slightly uneasy about it firing every mousemove.

What are other thoughts on which approach may make the most sense? Perhaps other ideas? Or hybrids of the above - (such as creating the hoveredBlock store described in approach 1, but still using elementsFromPoint when that changes to behave well with the inserter but run less often than mouse move)?

Thank you for taking the time to read through this wall of text and considering our options!

@youknowriad @mcsf any thoughts?
@vindl @noahtallen @jeyip @Copons @david-szabo97

@youknowriad
Copy link
Contributor

At first glance, I'd probably go with solution 1 personally. I believe we already track (or tracked) block hovering internally in the BlockListBlock component in an internal state, I'm fine with it in the block-editor state.

About the block inserter disruption, I wonder if this is related to the use of React events instead of DOM events (event bubbling is distrupted), so I'd consider trying event handlers using the "ref" in the BlockWrapper component.

@vindl
Copy link
Member

vindl commented Sep 21, 2020

Adding event listeners to every block may be unnecessary if there is not really a need for this sort of hovered block tracking elsewhere.

Could solution 1 be augmented so that blocks can opt into this?

Unrelated to any particular approach above - will this work for nested template parts? Do we have an easy way to determine the top level one with the above solutions?

@youknowriad
Copy link
Contributor

Do we have an easy way to determine the top level one with the above solutions?

maybe the blocks using controlled inner blocks. cc @noahtallen

@Addison-Stavlo
Copy link
Contributor Author

Do we have an easy way to determine the top level one with the above solutions?

In the above approaches the array of clientIds is ordered. So the template part inside another template part would be closer to the front of the list.

That does remind me of a similar problem we are trying to solve regarding how to tell if the selected block belongs to a template part. 🤔

@Addison-Stavlo
Copy link
Contributor Author

Could solution 1 be augmented so that blocks can opt into this?

There are definitely ways to make it opt in. However, if we are putting hoveredBlocks data in the block-editor store it may be a bit odd if it only tracks a subset of blocks?

@noahtallen
Copy link
Member

Unrelated to any particular approach above - will this work for nested template parts? Do we have an easy way to determine the top level one with the above solutions?

The inner block controller thing is one way to check a block to see if it is a parent template part. I'm not sure we actually need that in this case

In the above approaches the array of clientIds is ordered. So the template part inside another template part would be closer to the front of the list.

I imagine we could also do it the other way around too (e.g. array.push), so that the front of the array contains the higher-level blocks. Either way, we could just start searching from the side of the array that we want to start from.

In my opinion, I'd think that in the proposed designs, the "current" template part block would be active in the header area, which would be the most specific one (rather than the root one).

Could solution 1 be augmented so that blocks can opt into this?

(i agree with Addie, and had already written this up before that comment :P) Theoretically yes, but I don't know if it makes a big difference performance-wise? It seems to me that if we want to implement a block-editor-level store for the currently hovered block(s), then it should just work for all blocks. For example, it's not like we have to opt-in to getSelectedBlock on each block for that to be supported.

@Addison-Stavlo
Copy link
Contributor Author

About the block inserter disruption, I wonder if this is related to the use of React events instead of DOM events (event bubbling is distrupted), so I'd consider trying event handlers using the "ref" in the BlockWrapper component.

It doesn't seem to solve it (#25348 (comment)) but there are some other ways we could get around it.

@vindl
Copy link
Member

vindl commented Sep 22, 2020

Theoretically yes, but I don't know if it makes a big difference performance-wise? It seems to me that if we want to implement a block-editor-level store for the currently hovered block(s), then it should just work for all blocks. For example, it's not like we have to opt-in to getSelectedBlock on each block for that to be supported.

That makes sense. It was listed in the cons section of solution 1 and I tried to suggest a possible way to deal with it. But it sounds like it's not really a con after all, more like a feature/pro. :)

@Addison-Stavlo
Copy link
Contributor Author

Closing this as we have been moving forward with developing a solution on - #25348 - we can always come back to other approaches in the future if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective.
Projects
None yet
Development

No branches or pull requests

4 participants