Add template replace flow to template inspector #54609
Conversation
|
Size Change: +541 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
scruffian
force-pushed
the
fix/54562
branch
from
414aa8a
to
1888184
Compare
|
Flaky tests detected in b5ed84a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6330906056
|
I think that is nice for a follow-up, the main reason being: templates only have their name (via template hierarchy) to express what they re meant to do. In this use case we are replacing a template with another, and the user can revert that change, essentially letting them go back to the existing template. Patterns are better suited as "alternatives" to the templates that exist that you can revert back to. |
There was a problem hiding this comment.
Thanks for getting this started!
From what I understand it seems the template replacing is being done by switching the contents of the original template with a template pattern (or with another template? is there any difference between a template and a template pattern?), which gets saved as a user customisation. Is that correct?
Experience-wise it's a bit weird that if we want to go back to the original template we have to "clear customisations" (it's not obvious that replacing the template is a customisation imo)
Testing with the latest TT4 (downloaded from the repo) I'm getting Template part has been deleted or is unavailable in the front end on all the template parts of the newly replaced template. Not sure if that's a PR issue or a TT4 issue 😅
Are there any other themes this can be tested with? With TT3 the dropdown doesn't seem to appear for any template.
Also TIL about "Template Types" in patterns! Is this a new thing?
Afaik not many themes are using this yet because there is no flow for template replacement yet, so people just create patterns and expect the user to do the heavy lifting of editing the templates. Template types have actually been around for a little while! This works for the already existing flow when we create a new template. If you install TT4 and delete a template file from the template for it from the site editor, and it should let you choose from the patterns that have the search template type. With this PR the user doesn't need to remove the template manually (which is also a thing the site editor doesn't let you do unless it's a custom template) |
packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js
Outdated
Show resolved
Hide resolved
draganescu
changed the title
|
Since one of the ways to undo these changes is here: 
Should we also have the "Replace template" option there? If it's not a huge issue, I'm not sure if it's trivial to add. It just makes sense, and it's also one click less to get there. |
packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js
Outdated
Show resolved
Hide resolved
draganescu
force-pushed
the
fix/54562
branch
from
285f0f0
to
1e835b4
Compare
There was a problem hiding this comment.
I left some notes/suggestions for code improvements.
Considering that the templates have different meanings in classic WP. I think language can be improved to clarify what action does. It replaces the template content, not the template resolved by WP when viewing the site.
After selecting the replacement, I noticed that the modal became inactive; without any visual feedback, the screen seems frozen.
It would be nice to improve this experience in the final iteration.
Screencast
CleanShot.2023-09-26.at.16.23.31.mp4
packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js
Outdated
Show resolved
Hide resolved
| const entity = useEntityRecord( 'postType', postType, postId ); | ||
| const onTemplateSelect = async ( selectedTemplate ) => { | ||
| await entity.edit( { | ||
| blocks: selectedTemplate.blocks, | ||
| content: serialize( selectedTemplate.blocks ), | ||
| } ); | ||
| onClose(); // Close the template suggestions modal first. | ||
| onClick(); |
There was a problem hiding this comment.
Suggestion: The useEntityRecord also subscribes to a store; that data isn't used here. It might be better to call editEntityRecord directly.
Question: Do we need an error handle here?
There was a problem hiding this comment.
Done. I don't see editEntityRecord being used with an error handler other times but that doesn't mean its not a good idea :)
There was a problem hiding this comment.
Like this?
Although it's hard to say why edit would fail, the other instances also save where it makes sense to handle the error.
@scruffian we should also await the edit.
There was a problem hiding this comment.
@scruffian, you're correct. Edits don't make API request so it won't trigger an error. It's only createEntityRecord and saveEditedEntityRecord.
Sorry for the confusion.
packages/edit-site/src/components/sidebar-edit-mode/template-panel/template-actions.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js
Outdated
Show resolved
Hide resolved
Mamaduka
added
the
[Feature] Site Editor
|
In order to test this we need to download/install the tt4 theme, right? Also by downloading the theme right now, when I visit the site editor |
Yes, only TT4 has replacement templates available at the moment. Edit: Can't reproduce the issue with Footer template part. Are you testing with WP 6.3 or 6.4-beta1? |
There was a problem hiding this comment.
Thanks for the PR @scruffian! I took a first pass, but I need to better understand why we do so much to available patterns like injecting props. Are there any remaining blocking issues here?
Also it would be good to filter out the current applied content if we want to replace again. For example if we replace with writer index, we probably wouldn't want to suggest it again in the replace flow.
| { __( 'Clear customizations' ) } | ||
| </MenuItem> | ||
| ) } | ||
| { availablePatterns.length > 1 && ( |
There was a problem hiding this comment.
Probably better to add this check inside ReplaceTemplateButton and render conditionally, so as it's not needed for every consumer.
Also the check needs to change to something like !! availablePatterns.length because now if there is only one available pattern it can even lead to an empty DropDown. Noting that we would still the above check in order to avoid an empty DropDown.
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { store as editSiteStore } from '../../../store'; | ||
| import { TEMPLATE_POST_TYPE } from '../../../utils/constants'; | ||
| import { |
There was a problem hiding this comment.
Is there a specific reason for having this code in page-panels and not in template-panel?
There was a problem hiding this comment.
Historical reasons I think
There was a problem hiding this comment.
I think it's better suited to the panel folder is used. WDYT?
There was a problem hiding this comment.
Yeah I'll move it
packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js
Outdated
Show resolved
Hide resolved
| return ( | ||
| patterns | ||
| .filter( | ||
| ( pattern ) => ! PATTERN_CORE_SOURCES.includes( pattern.source ) |
There was a problem hiding this comment.
We probably can combine the consecutive filter calls, no?
|
@ntsekouras thanks for the review
The problem is that block patterns can contain template parts which are part of the theme. That means that they don't have a
I'm not sure about that. I think it's useful to see what options I have so I can go back to the one I had before, especially as I can make modifications to them. It would be useful to see which one is the current one, but I don't think the I think this PR is ready for another review. |
packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js
Outdated
Show resolved
Hide resolved
+1 |
|
Thanks for working on this one, it's an important one! |
mikachan
added
the
Backport to WP 6.7 Beta/RC
…anel/replace-template-button.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
There was a problem hiding this comment.
The code looks good to me and works as described in testing. Following the rounds of reviews the only outstanding thing is the performance:
- of the rendering in the modal of all the available patterns to replace the template contents with
- of setting the contents once a pattern was selected, the UI stutters and it's not immediately clear the action was not successful.
I would say let's follow up with optimisations but bring this in to note if there are other issues arising.
* Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
|
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: 283f4e7 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
* Add clearer labels and context to BlockPatternsSyncFilter (#54838) * Add help text & clearer labeling * Theme & Plugins * Font Library: use snake_case instead of camelCase on fontFamilies endpoint param (#54977) * use snake_case instead of camelCase on endpoint param * updating test * Fix output of Navigation block classnames in the editor. (#54992) * Block Editor: Avoid double-wrapping selectors when transforming the styles (#54981) * Block Editor: Avoid double-wrapping selectors when transforming the styles * Include space in the check * Don't display the navigation section in template parts details when a menu is missing (#54993) * Scripts: Properly use CommonJS for default Playwright config (#54988) * Fix path to `globalSetup` in default Playwright config Oversight from #54856 * `module.exports` * Fix default export usage * Add template replace flow to template inspector (#54609) * Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> * List View: Fix performance issue when selecting all blocks (#54900) * List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block * Fix left and right aligmnent in children of Post Template (#54997) * Fix left and right aligmnent in children of Post Template * Add align center styles * Fix image placeholder disappearing * Site Editor: Avoid stale navigation block values when parsing entity record (#54996) * Removed unwanted space from the string (#54654) * Update styles.js Removed unwanted space with translation * Update deleted-navigation-warning.js Unwanted space at the end of the string shows translation warning. * Update inspector-controls.js Unwanted space at the end of the string shows translation warning * Fix Deleted Navigation Menu warning string (#55033) * [Inserter]: Fix reset of registered media categories (#55012) * [Inserter]: Fix reset of registered media categories * convert `useInserterMediaCategories` to selector and make private * Try fixing the flaky 'Toolbar roving tabindex' e2e test (#54785) * Try fixing the flaky 'Toolbar roving tabindex' e2e test * Add a link in the comment * Fallback to Twitter provider when embedding X URLs (#54876) * Fallback to Twitter provider when embedding X URLs * Avoid processing empty urls when trying a different provider * Update `react-native-editor` changelog # Conflicts: # packages/react-native-editor/CHANGELOG.md * Based on the efforts in #51761, remove caps case from Template Part and prefer sentence case. As all instances of this string are stand alone, it's okay to have Template capitalized as it's the start of a sentence. (#54709) * Update pattern import menu item (#54782) * Update pattern import menuitem * Revert label * Image Block: Fix browser console error when clicking "Expand on Click" (#54938) * Patterns: Remove category description in inserter panel? (#54894) * Media & Text: Fix React warning (#55038) * Block Style: Display default style labels correctly in the block sidebar (#55008) * Site Editor: Do not display 'trashed' navigation menus in Sidebar (#55072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Image: Fix Lightbox display bug in Classic Themes. (#54837) * If current theme is not a block theme add a default value for $background_color and $close_button_color. * Set lightbox buttons' background & border to none on hover & focus * Change logic to support lightbox in classic themes * Update logic to avoid unnecessary calls * Add style fixes * Remove unnecessary code * Fix close button color --------- Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> * Latest Posts: add screen reader title text to Read more links and use alternative to excerpt_more filter (#55029) * In the editor: adds a screen reader text span with the post title in the i18n interpolator In the frontend: removes excerpt_more filter so we don't override themes and also replaces the default ellipsis with an accessible read more link * Removing "of" preposition in favour of a semi-colon. "Read more" is already translated so using a specifier to add it to the string * Fix Image block lightbox missing alt attribute and improve accessibility (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> # Conflicts: # packages/block-library/CHANGELOG.md * Patterns: Add category selector to pattern creation modal (#55024) --------- Co-authored-by: Kai Hao <kai@kaihao.dev> * Iframe: Fix positioning when dragging over an iframe (#55150) * Site Editor: Fix template part area listing when a template has no edits (#55115) * Alternative: Fix template part area listing when a template has no edits * Fix typos --------- Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: tellthemachines <tellthemachines@users.noreply.github.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Pascal Birchler <pascalb@google.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: mujuonly <muju.only@yahoo.in> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Michal <mmczaplinski@gmail.com> Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Kai Hao <kai@kaihao.dev>
| currentThemeStylesheet | ||
| ); | ||
| }, [ blockPatterns, restBlockPatterns, template, currentThemeStylesheet ] ); | ||
| } |
There was a problem hiding this comment.
This file is a bit confusing to me. It's unfortunate that we have to do all this work in order to get the available list of patterns for a given template.
- Isn't there a canonical way to retrieve block patterns?
- Do we really need the "inject" stuff?
We need to have more clarity in terms of APIs there. How should a developer know that he has to do all of this in order to make use of patterns?
cc @ellatrix @jsnajdr as you both worked on patterns before as well.
There was a problem hiding this comment.
There is the getAllPatterns selector in core/block-editor store that could replace most of this useAvailablePatterns hook. But it has one major difference: it also loads all the reusable blocks, aka user patterns, using the getReusableBlocks selector. But the Site Editor is apparently never interested in user patterns. Does it even make sense to use user patterns in Site Editor? Or are they intended only for the Post Editor.
If we have a version of getAllPatterns that doesn't return user patterns, we could use it at all four places in Site Editor where the code that gathers patterns from various locations is repeated.
There was a problem hiding this comment.
Do we really need the "inject" stuff?
Seems to me it was needed at the time when this PR was merged -- it was adding a theme attribute to all core/template-part blocks that didn't have a theme attribute. But then, one month later, @gziolo merged #55965 which added the theme fallback directly in the core/template-part block.
Today, I believe injectThemeAttributeInBlockTemplateContent could be removed. From more than one place.
There was a problem hiding this comment.
We would have to perform some extensive testing, but hopefully, injecting a theme attribute on the template level should no longer be necessary.
What?
Add template replace flow to template inspector - fixes #54562
Why?
Twenty Twenty-Four is exploring adding alternate template types for the home, index, single and page templates. While the effort in #51477 brings alternate template selection to pages, templates still do not have this capability.
There's currently not a method for someone to swap a template directly with another templateType.
How?
Add a new ReplaceTemplateButton component which opens a modal of available templates and lets a user pick the correct on.
Testing Instructions
Screenshots or screencast
swap-pattern-template.mp4
This work is heavily based on #51477