Size Change: +527 B (+0.02%)

Total Size: 2.57 MB

_{compressed-size-action}

Thanks for getting this PR up so quickly!

Note that this PR currently means that the controls for changing the template part design are no longer available. What could or should we do about that?

I'd vote to add them back in, especially now that template part editing is remaining in isolated "mode". I think if we're deliberately taking something away from users, there should be a pretty good reason. Is there one? Unless the design switcher itself is seen as having little or no value.

I get that editable fields might be a thing one day. Is it thinking too far ahead to leave that space blank in case that feature lands?

I'm sure @fcoveram will have better insight!

I'd vote to add them back in, especially now that template part editing is remaining in isolated "mode".

I think so, too. Basically for template parts I think we want kind of a hybrid, which is to say that we want template parts to show the content list and the design list when the template part block is selected within a template, just like how the right hand sidebar looks when you're in the isolated made and don't have any blocks selected:

I'll continue chipping away at this PR tomorrow (and/or next week) — I reckon the next task is to get the Design panel showing for template parts in contentOnly mode (possibly even the Advanced panel, too), and it'll then feel a bit better.

Just a quick screenshot before I wrap up for the day (note: I haven't committed this yet) — I think it should look pretty decent with the Design panel added:

I'll do some tidying and see if I can push something up tomorrow. I'm also thinking we will probably need to include the Advanced panel in this context, as otherwise there will be no way for a user to select Area, HTML Element, etc:

I think it should look pretty decent with the Design panel added:

+1 Thanks for looking into it.

I'm sure we'll hear about it if it doesn't work after merge 😄

Update: in d5dcb5f I've pushed a change to allow the slot and advanced controls for template parts (and technically for synced blocks, but they don't actually have those). The result is that we get a hybrid where we're still treating template parts as section blocks and therefore showing the content buttons to select inner blocks, but we also expose the Design and Advanced controls, allowing local changes for the instance of the template part. Here's how it looks:

I think this provides a pretty good balance, as it means we've (hopefully) kind of get the best of both worlds of content only controls, being able to click Edit Section to go to the isolated editor, while also allowing the advanced controls.

A byproduct of how I've done this is that some settings of blocks can be exposed in the inspector panel while selected. Like selecting a site icon or toggling settings for the site title. This felt pretty good to me, but happy to look into scaling / rolling this part back if it feels like too much.

I believe this is ready for review. Good things to test are that it's working correctly, but also smoke test with the experiments switched off to make sure that I haven't accidentally exposed anything here that shouldn't be.

Thanks!

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

I think the e2e failure is legit, I'll look into it. So the code here might not be final, but should be in a good place to see how it feels to use. If I don't get time to fix this PR properly today, I'll continue on with it on Monday.

Alrighty, I believe I've fixed it in a580576 by making this behaviour a bit more restricted. That is, we only inject InspectorControls and AdvancedControls if the root section is what's selected. That addresses the main case we want to handle. Here's how it's looking now:

Note that in the above video the template part itself will expose the Design and Advanced controls (in addition to the selectable blocks within Content). And then if you select blocks within a synced pattern, it does not expose settings and advanced.

The code change can likely be tidied up as we should ideally pass down the selectedBlockClientId instead of adding another useSelect, but just wanted to see if I could get it working properly first.

On second thoughts, I think refactoring might be best in a separate PR as some of the names in the parent component are a bit confusing. This should be ready for review now 🤞

we only inject InspectorControls and AdvancedControls if the root section is what's selected.

Nice! It's looking good to me:

Here's what I'm seeing:

When editing template parts in isolated mode, the template itself is in content only mode and I have to "Edit section" again to access the internals. This isn't the case for synced patterns. Should template parts work like synced patterns? Apologies if I've missed a convo/details on this. For my part, I'd expect to be able to edit the entire block tree.

When editing template parts in isolated mode, the template itself is in content only mode and I have to "Edit section" again to access the internals. This isn't the case for synced patterns. Should template parts work like synced patterns? Apologies if I've missed a convo/details on this. For my part, I'd expect to be able to edit the entire block tree.

This is an interesting one, thanks for testing @ramonjd! This isn't that the template part itself is in content only mode, it's that the template part contains a pattern for its markup, and because of that, to the user, it effectively looks like the template part is in content only mode. But it's actually because the template part contains a pattern.

I.e. if I go to view the source of my template part, I see a wrapper group block with the patternName:

<!-- wp:group {"metadata":{"categories":["header","wireframe"],"patternName":"core/simple-header-with-tagline","name":"Simple header with tagline"},....

So I think the question here is: what should we do about contentOnly mode when we're in the isolated editor? Should we switch it off entirely?

I think we probably should. We can even test it on trunk if we go to the Site Editor > Patterns > All template parts, and then select a header, you get this weird experience:

So yeah, I think the next step will be to disable it in the isolated editor, if that sounds reasonable to folks? I can dig into that idea next week. Given that this PR is changing what Edit Section does, I think it makes sense to roll it into the work in this PR (and that it feels pretty broken without it)

Let me know if anyone has other ideas, though!

his isn't that the template part itself is in content only mode, it's that the template part contains a pattern for its markup, and because of that, to the user, it effectively looks like the template part is in content only mode. But it's actually because the template part contains a pattern.

Oh, 😄 sorry that should have occurred to me

what should we do about contentOnly mode when we're in the isolated editor? Should we switch it off entirely?

I agree with you: that we should. I think it could get confusing for users to have to drill down twice. Just my 2c

Thanks for investigating 🙇🏻

Oh, 😄 sorry that should have occurred to me

No need to apologies, neither of us thought of it! I appreciate the 🦅 👀

Alrighty, I've pushed another change in fc6cdda that flags if we're in the isolated editor

I'm not sure if this is the best approach, but it's the best I could come up with on a Friday afternoon — I think we need a way to tell the block editor that it's in an isolated editor mode, and therefore to skip the logic that locks down unsynced patterns / treats them as contentOnly mode. That is, when editing template parts / synced patterns directly, we don't want to guard users from making edits.

To do so, I had to add a key to the editor settings and pass it from the editor... again, not sure if there's a better way to do it. But at least it seems to be working pretty well now (I'll likely need to update tests again). Here's another look at the state of this PR:

I'll continue on with this next week with fresh eyes, but as always, happy for feedback and ideas! (As an aside, I noticed the Back button in the document bar sometimes doesn't appear to work on first click... not sure what's going on there or if it's an existing bug)

This isn't that the template part itself is in content only mode, it's that the template part contains a pattern for its markup, and because of that, to the user, it effectively looks like the template part is in content only mode. But it's actually because the template part contains a pattern.

There is already an issue about this with lots of discussion - ContentOnly experiment: Template Parts with nested patterns appear locked when clicking 'Edit' from the toolbar.

This is one of the problems with the focus mode. The spotlight editing mode by comparison unlocks everything within a section. For focus mode, because it's such a separate experience it's a little harder to achieve that.

There is already an issue about this with lots of discussion - #73343 (comment).

Thanks for linking! The discussion there seems a little inconclusive, but I can add a comment over there, too. This PR is really just hacking on the idea of seeing how we could make the isolated editor feel good for template parts / unsynced patterns. I'm not sure we've achieved that, so consider this PR very much an exploration 😄

This is one of the problems with the focus mode. The spotlight editing mode by comparison unlocks everything within a section. For focus mode, because it's such a separate experience it's a little harder to achieve that.

Yeah, it's a challenging one. I've hacked this PR in to try to disable contentOnly mode altogether while in a focused mode / isolated editor to see how it feels. My hunch is that if someone is editing a template part or synced pattern in focused mode, maybe it's better that we don't really use contentOnly mode at all...

Happy to close out this PR if there's a better approach!

Yeah, it's a challenging one. I've hacked this PR in to try to disable contentOnly mode altogether while in a focused mode / isolated editor to see how it feels. My hunch is that if someone is editing a template part or synced pattern in focused mode, maybe it's better that we don't really use contentOnly mode at all...

Yes, that's my hunch too, but I think how it's achieved is the unclear part. I think there is a bigger picture in a sort of editor design mode that could be used for this, but it's too soon, so perhaps something temporary and non-controversial will work.

A few comments, but nothing major!

Fantastic, thanks for the comments and review Dan, that's super helpful! I'll work through these notes 👍

Flaky tests detected in 955301f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/20053167166
📝 Reported issues:

• [Flaky Test] As a user I expect my navigation to use the colors I selected for it #65427 in /test/e2e/specs/editor/blocks/navigation-colors.spec.js
• [Flaky Test] should open and edit templates correctly when active_templates experiment is enabled #73752 in /test/e2e/specs/site-editor/template-id-format.spec.js

Good feedback again @talldan, cheers. I think I've worked through it all now, so this should be ready for another look. Do let me know if there's anything I missed, though!

I just gave this a quick test run and it's working really well! Definitely keeping the design variations visible is a good idea.

One niggling thing that I noticed - probably unrelated to this changeset - is that it's effectively impossible to select the template part itself if its root container is a content block such as a Cover:

And the problem there is that the design options only appear if the template part is selected, not its root block. I wonder if template part and root block should be equivalent to each other for this purpose?

One niggling thing that I noticed - probably unrelated to this changeset - is that it's effectively impossible to select the template part itself if its root container is a content block such as a Cover:

Oh, good observation! I'd probably consider this something to dig into in subsequent PRs, but I wonder if it'd be worth trying to either:

• Make clicking outside of the buttons in the Content panel select the parent / containing section block (in this case the template part)
• Or make clicking the button in the Content panel for block a toggle so that if it's already selected, clicking it a second time will select the parent / containing block

Not too sure, just spitballing ideas for how to make it easier to select the outer wrapper.

I'm not sure we should try to expose the parent's tools (design, advanced) directly to the inner block unless we really have to, though, as it could get a bit tangled... but maybe it's not as complex as I fear. In any case, good ideas to play around with!

I'm a bit time-limited this week, so I might park working on figuring that out for now. Let me know if there's anything anyone thinks should still be addressed in this PR, though.

• Or make clicking the button in the Content panel for block a toggle so that if it's already selected, clicking it a second time will select the parent / containing block

This one sounds more intuitive to me. But yeah definitely something for another PR!

Just added a comment. If there aren't any objections, I can look into merging this tomorrow. (Also happy to keep iterating if there are any concerns, of course)

If there aren't any objections, I can look into merging this tomorrow.

Let's do it! Great stuff