Skip to content
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

Adds "Template Parts" command to site editor #61287

Merged
merged 5 commits into from
May 9, 2024

Conversation

bacoords
Copy link
Contributor

@bacoords bacoords commented May 1, 2024

What?

Adds command to directly access your Template Parts while in the site editor.

Why?

This was actually a command that existed but was removed in #52817 as the Patterns and Template Parts UI were somewhat merged. However, since Template Parts are still a distinct entity and not always discoverable as something you'd find under "Patterns", this PR adds the direct link via command palette.

Closes #60443

How?

New command was added mostly matching the original but deep linking directly to template parts instead linking to Patterns and requiring an additional click to see.

Testing Instructions

  1. Open the site editor.
  2. Click cmd-k to trigger the Command Palette
  3. Start searching for Template Parts
  4. Select Template Parts and confirm you're navigated to the correct screen

Screenshots or screencast

Screenshot 2024-05-01 at 7 25 52 AM

Needs feedback on:

  • Currently the command loads in the site editor but not the page editor because only admins can edit the template parts. To load in the page editor, it needs to be checking user permissions before registering the command, but I wasn't seeing a clear way to do that.
Copy link

github-actions bot commented May 1, 2024

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: bacoords <bacoords@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
Co-authored-by: markjszymanski <markjszymanski@git.wordpress.org>
Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
Co-authored-by: mattreport <mattmm@git.wordpress.org>

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

@bacoords
Copy link
Contributor Author

bacoords commented May 6, 2024

@annezazu @colorful-tones Do you know who I would @ to get labels and feedback on this PR? (Commands / Core Commands)

@colorful-tones
Copy link
Member

@annezazu @colorful-tones Do you know who I would @ to get labels and feedback on this PR? (Commands / Core Commands)

I've assigned a few folks for PR review that I feel like could help move this forward. I think this would be a nice addition for WP 6.6 and would be happy to triage it onto the board, but I don't want contributors to feel like I'm adding more work. For now, I'll leave it off.

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

This PR makes sense to me, but I'd like to pass on a few points to move forward.

  • I think this command should be added to site-editor-navigation-commands.js instead of admin-navigation-commands. Then you may be wondering why the "Patterns" command exists in admin-navigation-commands, but I think the Patterns command should also be moved to site-editor-navigation-commands.js. Please refer to the explanation of #54133 for the reason why it currently exists in admin-navigation-commands.
  • This command should only be enabled if the template part actually exists. You will probably need to get the template parts via the getEntityRecords selector and check their number.
  • You could reuse the useIsTemplatesAccessible() hook to check if the site editor is accessible.
  • Finally, I think here is where this command should be added. The Patterns command mentioned above should also be moved here via a separate PR.

If there is anything you do not understand in these explanations, please feel free to ask 👍

@bacoords
Copy link
Contributor Author

bacoords commented May 6, 2024

Thanks for the help on this!

I think this command should be added to site-editor-navigation-commands.js instead of admin-navigation-commands.

I originally wrote it this way, but then the command is displayed in the initial blank state when you open the command palette in the site editor. I'm wondering if that's the ideal behavior and if we want that? (I'm not personally against it, but I assume since it's not in the main navigation on the left-hand sidebar, maybe it's not wanted in the initial command palette state?)

This command should only be enabled if the template part actually exists. You will probably need to get the template parts via the getEntityRecords selector and check their number.

Can you clarify? I believe that a user can create new template parts regardless of whether any exist in the theme already, as long as it's a block theme.

You could reuse the useIsTemplatesAccessible() hook to check if the site editor is accessible.

From what I can tell, that hook only tells me if the current user can read templates. An editor-level user can technically read templates but aren't allowed to access the site editor, so in my testing, the hook didn't prevent the command from loading for non-admin users.

I'm wondering if these types of commands should rely on a slightly different hook that checks .canUserEditEntityRecord( 'templates' ) because in the latest Gutenberg right now editors see all of these options (Templates, Patterns, etc) in the command palette and they all send the user to a "Sorry, you are not allowed to access this page." screen. (This appears to be new behavior and is not in WordPress 6.5: Playground Link)

The Patterns command mentioned above should also be moved here via a separate PR.

I've opened this as a separate PR so we can move these forward together #61416

Thanks again for the feedback.

@t-hamano
Copy link
Contributor

t-hamano commented May 7, 2024

As stated in this comment, in order to proceed with this PR, it seems necessary to resolve #61419 first.

@t-hamano
Copy link
Contributor

t-hamano commented May 8, 2024

but then the command is displayed in the initial blank state when you open the command palette in the site editor.

This is probably because you tried adding it to a command that has a 'site-editor' context? By default, a command should not be displayed if it has no context.

I believe that a user can create new template parts regardless of whether any exist in the theme already, as long as it's a block theme.

After running this command, I expect the "All template part" menu on the Patterns page to be focused. However, if the theme does not have any template parts and no new template parts have been created yet, this menu itself will not be displayed. This is why I suggested checking if the template part actually exists.

I would like to suggest using the getNavigationCommandLoaderPerTemplate() function and changing it as follows.

const getNavigationCommandLoaderPerTemplate = ( templateType ) =>
	function useNavigationCommandLoader( { search } ) {
		// ...
		const commands = useMemo( () => {
			// ...
		}, [ isBlockBasedTheme, orderedRecords, history ] );

		if ( records.length > 0 && templateType === 'wp_template_part' ) {
			commands.push( {
				name: 'core/edit-site/open-template-parts',
				label: __( 'Template parts' ),
				// ...
			} );
		}

		return {
			commands,
			isLoading,
		};
	};
@bacoords
Copy link
Contributor Author

bacoords commented May 8, 2024

Thank you for the feedback! A few updates:

  • Technically the "All Template Parts" screen still loads if you don't have any template parts, but because the sidebar navigation just shows "Patterns", I agree it's a confusing experience and that using getNavigationCommandLoaderPerTemplate is the cleanest approach in the context of this PR.
  • I've added the command to the getNavigationCommandLoaderPerTemplate but ended up needing to put it inside the useMemo to make sure it wasn't loading the command multiple times and dependent on the records. That did require a little bit of reconfiguring the return and using the memo dependencies correctly, but I believe it's all functioning correctly now.

There is still a separate permissions issue that these template parts now load in the command palette for "Editor" level users, but if they select one, it send them to an error page. If it makes sense, I can open a separate PR for that.

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

  • ✅ Works both inside and outside of the site editor.
  • ✅ It also works with classic themes that support template parts.

There is still a separate permissions issue that these template parts now load in the command palette for "Editor" level users, but if they select one, it send them to an error page. If it makes sense, I can open a separate PR for that.

Yes, I also noticed this issue in #61444.

While creating this PR, I also noticed that individual pages, individual templates, and individual template parts were exposed to non-admin users via the command palette. These are caused by the fact that there is no permission check in the first place, so I would like to fix it in a follow-up PR.

However, I haven't started creating a PR yet, so if you're interested in working on it, I'd be happy to review it.

@t-hamano t-hamano merged commit dbd6bda into WordPress:trunk May 9, 2024
61 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.4 milestone May 9, 2024
@ajlende ajlende added [Type] Feature New feature to highlight in changelogs. [Package] Core commands [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") labels May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Package] Core commands [Type] Feature New feature to highlight in changelogs.
4 participants