Try adding a 'spotlight mode' type effect when template part or child is selected. #25656
Conversation
|
Any thoughts on how this feels @shaunandrews @jameskoster @dubielzyk @olaolusoga? It will be overridden if 'spotlight mode' becomes enabled, but I don't think thats necessarily a bad thing. |
Addison-Stavlo
requested review from
noahtallen,
vindl,
mattwiebe,
Copons,
david-szabo97,
jeyip and
shaunandrews
Addison-Stavlo
changed the title
|
Size Change: +201 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
Addison-Stavlo
added
[Feature] Full Site Editing
Needs Design Feedback
|
I quite like it. We're not communicating why we're entering spotlight mode to the user, but perhaps that's fine. Would this also trigger if you have nested template parts? |
|
I think spotlight is the way to go, but the finer details of what happens when inner blocks are selected, or when multiple instances of a single template part exist on the canvas, needs some refinement. In #25452 I worked on a couple of crude prototypes that we can perhaps discuss here. In this latest iteration:
|
@dubielzyk - Good question, it is a little odd right now the nested template parts are spotlighted together regardless of whether the parent or child is selected. I will try to make this better so that only the child is spotlit when it is the one selected.
@jameskoster - I think it is fair to follow up and reiterate regarding how to handle duplicates of the same template part that aren't selected. Currently, it isn't possible to have 2 of the same template part on one page and that bug is in the process of being worked out technically. But should that prevent us from adding the ability to spotlight for the selected template part in the meantime? If we agree that the opacity changes are the direction forward, then a small scoped change like this would be a good first step.
The first outline already exists as a subtle gray border and I agree it does work well with the template part spotlight mode. The second would be a simple change in the blocks editor files, and the third would require some changes in the block-list/wrappers but should be doable as well if we determine that is the way to go. Both of these I could see as potential follow ups after we are able to successfully render 2 of the same template parts on one page. |
Seems fair to me :) |
|
This feels nice. If we can make the logic work for nested template parts, I'd love to see this merged. |
Addison-Stavlo
requested review from
ellatrix and
youknowriad
as code owners
|
Nice, the nested case works well. I also appreciate how "spotlight mode" takes precedent over this, so it still works the same as before with template part blocks. |
There was a problem hiding this comment.
Nice! I like how this works.
before merging, i'd love to see if we could disable it for non-FSE sites (so we can test it out without affecting plugin consumers).
Besides that, from a code point of view, I dislike how we are tightly coupling one block type with the general block list. I wonder if there's a better way to handle it -- perhaps adding is-active-template-part to the template part block's edit method (or just adding the right CSS properties to the template part's edit div)
I can see how template-part-highlighting needs to be more general though. I'm not thinking of any alternatives to that approach off the top of my head though.
I understand that concern. However, we do need to access this area to handle adding the While it is 'just one block type', it could be extended to more block type if desired in the future (post content, etc.). So maybe rebranding these to more generic terms than 'template parts' would make sense? 🤷♀️
We could do that, although it would separate the code used to achieve this functionality into 2 separate packages which to me seems much less ideal.
Im not sure I follow what these right CSS properties would be? Opacity is a bit of a complex case in that it bleeds down, so just setting In the end, using the block-list / wrappers seems necessary for implementing an editor wide content opacity control and while we may be referencing 'one specific block type' in the general list component that seems much more manageable than splitting up the functionality and style rules between multiple packages. |
|
Thanks for the detailed explanation!
I would be in favor of this then |
Any ideas on names? 😆 |
Addison-Stavlo
force-pushed
the
try/spotlight-mode-for-template-parts
branch
from
ca13ce1 to
1646269
Compare
There was a problem hiding this comment.
I really like the latest round of updates to this, the code looks much more polished now! ✨ Gave it another spin and everything still works as expected on my end. I also made sure that these changes are not affecting the post editor.
I added a few optional naming suggestions.
| * @param {boolean} ascending Order results from bottom to top (true) or top to bottom (false). | ||
| * @param {Object} state Editor state. | ||
| * @param {string} clientId Block from which to find root client ID. | ||
| * @param {string|string[]} blockName Block name(s) to filter. |
There was a problem hiding this comment.
Nitpick: should we update this to blockNames then? Reading blockName.includes threw me off at first.
There was a problem hiding this comment.
🤔 Im not sure tbh. The original use is a string of a single blockName and this is still supported. Since it can either be a string or an array of strings, whether we use blockName or blockNames it will still read a bit funky somewhere. ( like name === blockNames ) 🤷♀️ At least the blockName.includes is nested in an if ( isArray( blockName )
| } = select( 'core/block-editor' ); | ||
|
|
||
| // Determine if there is an active entity area to spotlight. | ||
| const activeEntityBlockId = __experimentalGetActiveBlockIdByBlockNames( | ||
| getSettings().__experimentalSpotlightBlocks |
There was a problem hiding this comment.
Would it make sense to make this a default parameter value if it's not passed? Then we could also shorten the name of the selector to __experimentalGetActiveEntityBlockId, which better aligns it with surrounding variable names that we are using. I guess it depends on how often we anticipate this selector will be used outside of this functionality.
Also I'd probably rename __experimentalSpotlightBlocks to __experimentalSpotlightEntityBlocks to avoid potential confusion with regular spotlight feature.
There was a problem hiding this comment.
Would it make sense to make this a default parameter value if it's not passed?
I guess it depends on how often we anticipate this selector will be used outside of this functionality.
Yeah, I guess that 100% depends on how this selector might be used. For our purposes, that would make sense! However, without that change this selector is more general and could be used for other purposes. Whether it would be, I really don't know. 🤷♀️
Also I'd probably rename __experimentalSpotlightBlocks to __experimentalSpotlightEntityBlocks to avoid potential confusion with regular spotlight feature.
👍
Description
Re-exploring the idea of adding a 'spotlight mode' effect on template parts.
Here, we add a new class to pass down from the block-list if a template part or any of its children are selected. And some style rules similar to the current focus-mode.
This works as a default behavior for template part/child selection and the styles are overridden if 'spotlight mode' is enabled.
How has this been tested?
Screenshots
Types of changes
Checklist: