WIP - Try/hovered template part tracking #25382
Conversation
| export function documentSettingsDropdown( | ||
| state = { hoveredEntities: [] }, | ||
| action | ||
| ) { |
There was a problem hiding this comment.
This extra level may not be necessary, but I was thinking it may be helpful if we need other items tracked in central state than just the hovered items. (Ex - Dectecting if a template part has an inner block selected may be easier from the template parts end, since we could dispatch the selected template part block's ID to this store given a hasSelectedInnerBlock in the edit component itself.)
Also, this may not be the correct store.. perhaps a different store or a new one specifically for the document settings dropdown may make more sense?
There was a problem hiding this comment.
Yeah I could definitely see needing a store for document settings at some point. I also agree that it probably isn't the right store. Depending on how things go, I think a nested reducer in the editor stores might be the most obvious place 🤔
| <div | ||
| onMouseEnter={ () => addHoveredEntity( clientId ) } | ||
| onMouseLeave={ () => removeHoveredEntity( clientId ) } | ||
| > |
There was a problem hiding this comment.
This didn't work on the BlockWrapper, the onMouseEnter worked there, but onMouseLeave was getting overridden.
Adding this ad-hoc doesn't seem bad given we don't have many entities we need to highlight in the document settings dropdown. However, if we did wan't this functionality to be more reusable maybe we could add these handlers via a supports hook similar to the style hooks? 🤔
| const { hoveredEntityIds, getBlock } = useSelect( ( select ) => { | ||
| const { getHoveredEntities, getBlock } = select( 'core/block-editor' ); | ||
|
|
||
| return { | ||
| hoveredEntityIds: getHoveredEntities(), | ||
| getBlock, | ||
| }; | ||
| } ); | ||
|
|
||
| const hoveredName = | ||
| getBlock( hoveredEntityIds[ 0 ] )?.attributes.slug || ''; | ||
|
|
There was a problem hiding this comment.
This is all that was changed in this file, please disregard the extra linting changes below.
Similarly, we would try to update this to show the title instead of the slug where available.
|
Size Change: +306 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
Overall I am thinking I like this approach better than the previous, just given that we aren't adding an unnecessary handler to every block and our parsing on the header side can be a bit more simple and run less often. |
| 'Add block', | ||
| 'Generic label for block inserter button' | ||
| <> | ||
| <div className="edit-site-header">{ hoveredName }</div> |
There was a problem hiding this comment.
Again this just just the temporary ugly thing to show the hovered name for now. 😆 lines below this are unchanged.
There was a problem hiding this comment.
It's so hard to choose! Some thoughts:
- I do like that this is a more limited change.
- I don't think the block-editor store makes sense here. If we are making a publicly accessible API for block hover states, then I definitely think that the other PR is better in that regard.
- I'm not a big fan of having to add hover handler wrappers to every block we want to add this too..
I wonder if a combination would be better? Like letting blocks opt-in to hover tracking, and then using the store from the other PR?
I think if we do end up going this route, it might make sense to just use some reducers/actions in edit-site for the time being. (Since we aren't designing it around a general use case, but something specific to us..)
| @@ -1106,3 +1106,17 @@ export function setHasControlledInnerBlocks( | |||
| clientId, | |||
| }; | |||
| } | |||
|
|
|||
| export function addHoveredEntity( clientId ) { | |||
There was a problem hiding this comment.
I feel like we want to avoid mingling the "entity" concept with the block editor. I think the idea of an "inner block controller" is similar (the "inner block controller" is more of an "entity block controller", but the entity stuff is outside of the scope of the block-editor package.)
Agreed.
How many blocks do we really need to add this to? Template Part and maybe Post Content seems to be the consensus currently, so it doesn't seem too horrible.
If we did see the need to make this more reusable, I could see allowing blocks to 'opt-in' through the supports field in the blocks.json. Then these handlers could potentially be added with a hook similar to the style hooks. Would that make sense as a way to create the opt-in? but is that something we should currently be pursuing given there aren't plans to use this sort of thing beyond 1 or 2 blocks?
Im not following why the store from the other PR would be more acceptable? There doesn't seem to be much of a need for vague hover tracking, and if it is opt-in only then coming across selectors like |
I think the thing is, when we have a selector My main point is that if we created I think it's different if this was instead I think if we want to be general and do a block-editor-level store to contain this information, then we should go all in, and track every block, so that it actually is general for arbitrary blocks. I think if we want it to be not general and only specific to what we need, maybe it makes sense to have it actually be tightly-coupled with the site editor store for now. Does that make sense at all? |
Totally! Thanks for elaborating. Something like Im happy coupling it to the site editor for now. Going all in and adding tracking to every block could be cumbersome, unnecessary, and/or stall us from moving forward as it is a bit of a larger change that effects a lot more than just the template part and the site editor. While it is neat to be able to track the all hovered items from external components, I don't think there is a large demand for this ability? |
🤷 Not sure!
Yeah, I think I am too. The only unknown is: I'm not sure how common or "best practice" it is to hook blocks to the editor UI like this.
true! How about Maybe.. |
Its not. There may be some grief over it, but it is non-breaking and compatible with both editors. 🤷♀️ Maybe we could set up our own little store
Sounds good to me! |
|
The behavior here works as described in Chrome, Edge, Safari, and IE11. ⭐️ There's a little bit of wonkiness in Firefox where initially hovering a template part causes the refresh icon in the browser toolbar to quickly toggle between "refresh" and "cancel refresh". 🤷♂️ Probably not your PR, but it is a little distracting. |
I don't have much to add to the conversation, but I agree with the direction of the PR.
Agreed! |
|
Closing this exploration, as we will work out the solution in #25348 |
Description
Exploration to go along with #25320 (alternative approaches discussed in #25469 - more detail on this approach there as "Solution 2").
How has this been tested?
Screenshots
Types of changes
Checklist: