-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Template Parts: Fix the template part replace flow #59883
Conversation
Size Change: +43 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
…e to the template
31f8411
to
43b55f7
Compare
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @altivero. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I agree that there's some confusion here but I'd love to have more opinions on this, because I can see both approaches as useful. (For instance if you want to share the same template part across patterns, you might to replace the template part rather than its content). |
I do remember updating the patterns part of the replace flow here in the past to do the opposite - Ensure replacing a template part using a pattern doesn't update the existing entity, so that might add some historical context. |
This is definitely tricky. I was just chatting with @jameskoster about this, about how we are intentionally omitting things like color settings directly on the template part itself, exactly because those are locally applied, not globally to the template part you selected. I would think that the same instinct applies here, that if we surface transforms for the template part itself, then those should apply to the template part itself, meaning across. The flipside, making a local-only change also feels slightly less common than making a global change, at least when you are interacting with a template part. Not a strong opinion, and would also like to hear more thoughts. |
It's even more subtle here though, because while applying a local change (changing the referenced template part), you're also making the decision to reuse that referenced template part across multiple templates (global change). |
I am unclear what the terms global and local mean in this context :) |
This is a good example of why I'm not particularly fond of full inline editing for global/synced entities. It never quite feels clear whether your changes will be applied locally or globally. I'd be interested in trying something like the Figma component model. IE when you select an instance of a synced entity, only local edit options (IE overrides) are surfaced in the Inspector. There would also be prominent (possibly toolbar level) links to:
In terms of how to move forward here, perhaps we can look at the new Inspector designs for guidance. The concepts there cover these scenarios. Here are some mockups:
|
Requiring TPs to be edited in isolation would solve some of these issues, but it would come at the tradeoff of not being able to see the full context of the page. Additionally, this would solve the issue only for template parts; even though synced patterns cannot yet be replaced with something else in a similar manner. Are there other options? |
For me the main thing is to add a little friction to the experience so the user is able to better understand when they're editing globally. There could be three toolbar actions;
|
Lots to like in your design.
I'd echo, this is the main thing to figure out. With these three buttons, the block toolbar becomes slightly overwhelming: Compare to what's shipping: Since in what's shipping, the Edit button already takes you to focus mode, whereas another click on anything inside simply edits it inline, I wonder if we shouldn't just keep that behavior, and those labels, but just invoke the inline-editing-dimming that you have as soon as you click any item inside? What would the "Replace" button do, in your concept? |
Which feels like a costly trade-off.
What do you think of this proposal (independent of content-only inspector controls when a template part is selected)? #59970 |
I like the simplicity. Where does "edit in isolation" go? I still think that's something we need to make accessible for everything. But perhaps that can be in the ellipsis menu? #45264. |
Allow you to swap with another pattern from the same category. Doesn't have to be so prominent – it could be an icon button, or live in the ellipsis menu.
I'm not convinced this is enough friction alone, but it could be a place to start and iterate from. I think a double-click could be worth trying too, but on balance I still prefer the dedicated 'Edit' button in the toolbar. @richtabor seems our concepts are virtually the same. Like @jasmussen says we just need to figure out where the edit buttons live. I tend to think that editing in isolation is less likely to be the experience you want in this flow, so I reckon that could live in the ellipsis menu. |
Note this is a bit off topic for this PR; perhaps we should continue this on #59970?
I don't mind the "Edit" button in the toolbar like it is now, perhaps engaging the inline editing, as explored in #59970.
I do too. Should there be a different way to engage in isolated? Maybe a more explicit control in the block options? |
Sounds like we're all actually in agreement, but just to summarize my read of next steps, hopefully we can unblock this.
Figma has this option for editing in isolation, not a 1:1 but close enough: You right click a symbol and choose "go to main component". Is "Go to Template Part" or "Go to synced pattern" a better term? Or should we keep "Edit in isolation"? |
"Go to $entity-name" might be nice. But names can be very long.... "Go to Centered Header Menu with Search" would be an awkward menu item – we'd probably need to truncate. On balance I lean slightly towards something like "Edit in isolation", mostly because it'll be consistent across, and aligns verbally with the primary 'Edit' action. Other potential options:
|
Edit in isolation, in the ellipsis menu, sounds like a good next step. |
Thanks for the discussion. It would be helpful if we could get some clarity around this PR. Should I close it? |
There are quite a few moving parts to connect, notably:
I still think the principles outlined in #59883 (comment) make sense. IE: The tiles only appear when you're editing the template part, and selecting a different tile replaces the contents of the template part you're editing. So with that said, I think the behavior on trunk is wrong and this PR moves us in the right direction. One change I'd suggest is to rename the "Replace" panel, since the behavior is no longer a replacement. The designs in #59689 suggest a "Design" panel for this purpose, which would contain sections where you can change the design/layout, style variation, etc. We might begin to install that here: What do you think? |
I like the "Design" term. Can be separate, but the big drop shadow under these seems out of place. I wonder if it shouldn't be a thin border, a la what we're discussing here: #60110 (comment) |
Agreed. We probably don't need the titles either, like the pattern categories in the Inserter. |
I created a PR for the text change here: #60156 |
Just chiming into this discussion to say I don't think this is an engineering problem to solve in this PR. The main issue I see, as others mentioned, is that the same action (replace) results in 2 different outcomes (replace contents or replace reference). This is very confusing as we don't know if the user wants to update the template currently being edited or all the templates using the template part they're replacing. I would err on the side of not doing global changes when not sure. In this specific PR replacing the contents of a template part with the contents of another template part can lead to a library filled with the same duplicated template part. While it is possible to revert or step back through revisions, it's hard to know which one to edit because we lack a way to communicate where reusable content is reused (synced patterns, template parts, navigation). I think the solution has to come from creating a good UX which allows the user to opt between updating the template part across all templates that use it, or updating the current template's reference for a specific template part. Using one action (replace) is so far very confusing. |
Thanks for the discussion on this. I am trying a new idea in #60203 |
Can we please try to link to a pertinent Issue in a Pull Request's description? I'm going down the rabbit hole of trying to figure out features and it is super difficult as it is. 😅 This PR was closed and another one opened, which also does not reference an Issue it is trying to solve. 🤦
|
Perhaps we were trying to address this? #59970 |
What?
Changes the template part replace flow so that it replaces the contents of the template part itself rather than just the template's reference.
Why?
The current template part replace flow doesn't replace the contents of the template part, it replaces the reference to the template part in the template. This means that only the current template is updated. I think users would expect that this would instead update the template part itself.
How?
This reuses the approach in
packages/edit-site/src/components/sidebar-edit-mode/template-panel/index.js
, which copies the blocks from the pattern and puts them into the entity.Testing Instructions
The best way to test this is to open two different templates that use that template part (for example the home page and a posts page).
Screenshots or screencast
Screen.Recording.2024-03-14.at.16.40.17.mov