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

Make the Preview Menu extensible #31984

Closed
wants to merge 22 commits into from

Conversation

delawski
Copy link
Contributor

@delawski delawski commented May 19, 2021

Description

This PR introduces a new <PluginPreview> component to the @wordpress/block-editor package. It allows for defining a custom "Preview" menu item along with optional content that is rendered instead of the VisualEditor.

Thanks to that, plugin authors can extend the "Preview" menu and provide optional custom content previews.

This PR is based on #25430 proposed by @mreishus. It addresses @jorgefilipecosta's feedback pointing out that the new component added to @wordpress/block-editor should not depend on the @wordpress/edit-post package.

In the original implementation we were using a deviceType and a setDeviceType action from the core/edit-post store. Right now, they are passed to the slot fill by the VisualEditorOrPluginPreview component (that lives in the @wordpress/edit-post package).

There's one additional difference between this PR and the original one. The custom preview (PluginPreview component children) is optional now. If there is no custom preview provided, selecting a custom "Preview" menu item will not affect the VisualEditor. Instead, plugin authors may rely on the onClick event handler for providing custom functionality. This may be very helpful in the case of plugins that provide external previews (e.g. paywalled content, search results, or AMP preview as proposed in ampproject/amp-wp#5943).

How has this been tested?

  • Create a new block editor plugin as follows:
import domReady from '@wordpress/dom-ready';
import { __ } from '@wordpress/i18n';
import { PluginPreview } from '@wordpress/block-editor';
import { registerPlugin } from '@wordpress/plugins';
import { external } from '@wordpress/icons';

domReady( () => {
	const PluginPreviewTest = () => (
		<>
			<PluginPreview
				name="preview-custom-1"
				title={ __( 'Custom Preview 1' ) }
			>
				<h1>
					{ __( 'Custom Preview 1 Content' ) }
				</h1>
			</PluginPreview>

			<PluginPreview
				name="preview-custom-2"
				title={ __( 'Custom Preview 2' ) }
				onClick={ ( event ) => console.log( event  ) }
			>
				<h1>
					{ __( 'Custom Preview 2 Content' ) }
				</h1>
			</PluginPreview>

			<PluginPreview
				name="custom-action"
				title={ __( 'Custom Action' ) }
				icon={ external }
				onClick={ ( event ) => console.log( event ) }
			/>
		</>
	);

	registerPlugin( "plugin-preview-test", {
		render: PluginPreviewTest,
	} );
} );
  • Go to block editor and confirm there are 3 new items in the "Preview" menu:
    Screenshot 2021-05-19 at 11 38 23
  • Click the "Custom Preview 1" menu item and confirm that the block editor content has been replaced with a custom preview
  • Click the "Custom Preview 2" menu item and confirm that besides changing the editor content, a click event object has been logged to the console
    Screenshot 2021-05-19 at 11 40 19
  • Click the "Custom Action" menu item and confirm the editor config has not been changed while a click event has been logged to the console.

Screenshots

Screenshot 2021-05-18 at 20 50 27

Types of changes

New feature (non-breaking change which adds functionality)

Fixes #25309
Relates to Automattic/wp-calypso#42896
Relates to ampproject/amp-wp#5943

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@delawski
Copy link
Contributor Author

cc @simison @westonruter @pierlon

@delawski delawski force-pushed the add/preview-menu-slot branch from e25140c to 0a205de Compare May 19, 2021 10:17
@gziolo gziolo self-requested a review May 19, 2021 16:35
@gziolo gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience [Type] New API New API to be used by plugin developers or package users. labels May 19, 2021
@gziolo gziolo requested a review from jorgefilipecosta May 19, 2021 16:35
@simison simison requested review from jasmussen and jameskoster May 19, 2021 19:45
@jasmussen
Copy link
Contributor

Thanks for working on this! It's non-trivial to test, I got this far:

after

... but have to move on to some other tasks, so can't explore the intricacies too deeply. For that reason, thank you for including great screenshots as well.

High level, I still think it makes a lot of sense for us to allow this menu to be extensible, so that plugins can add custom preview interfaces exactly like what is outlined for AMP.

Looking at the current API, though, it seems like this would plugins to add anything there — not just menu items, which I fear is likely to be abused to show all sorts of irrelevant things. Is there any way we can rein in the API to only allow menu items in a very specific format? Ideally plugin developers should not have to know about which components to use to make a menu item look correctly like a menu item. Considering we can always expand this in the future, what is the least we can start with? Label & onclick action? Label & external link action? Do we need to allow custom icons & subgroups?

The less we can start with, the better we'll know where to improve and expand upon it in future iterations. What do you think?

@gziolo
Copy link
Member

gziolo commented May 21, 2021

@jasmussen, should we follow the same path as with the PluginSidebar with the automatic menu item injection? We could even go one step further and don't expose the menu item fill at all, so all you would have to do is to define PluginPreview and it would know where to render the menu item?

By the way, isn't the name "preview" too generic?

@jasmussen
Copy link
Contributor

We could even go one step further and don't expose the menu item fill at all, so all you would have to do is to define PluginPreview and it would know where to render the menu item?

I like the sound of that. While as noted I think there's a place for this feature, I like for APIs to be mostly agnostic in their implementation, so if we were to theoretically move the preview dropdown somewhere else in the interface, the API would still work and make sense.

By the way, isn't the name "preview" too generic?

Good point, especially as I've seen recent conversations about renaming it to just "View".

@delawski delawski force-pushed the add/preview-menu-slot branch from 9189a38 to f4e401b Compare May 24, 2021 08:29
As per the review feedback, there seems to be no need to expose the Preview menu item fill at all. It should be sufficient to expose the title, icon, and click handler. This way we "protect" the Preview menu drop-down from injecting arbitrary, irrelevant elements into it.

Thanks to that, the entire custom previews implementation could be simplified. It's now mostly contained within the `PluginPreview` component itself.
@delawski
Copy link
Contributor Author

@gziolo @jasmussen Thanks for your feedback!

We could even go one step further and don't expose the menu item fill at all, so all you would have to do is to define PluginPreview and it would know where to render the menu item?

I like the sound of that. While as noted I think there's a place for this feature, I like for APIs to be mostly agnostic in their implementation, so if we were to theoretically move the preview dropdown somewhere else in the interface, the API would still work and make sense.

I like this approach, too. I think having a similar API as the PluginSidebar makes much sense. Can you please review the revisited implementation (after f949647)?

By the way, isn't the name "preview" too generic?

Good point, especially as I've seen recent conversations about renaming it to just "View".

I haven't done anything about this, yet. Do you think preview won't last and we should go with a more generic view instead? Or should we leave it for now?

@delawski
Copy link
Contributor Author

Note that the PR description has been updated to reflect the changes introduced in f949647 (i.e. that there is now just one new component, the PluginPreview).

@leutrimhusaj
Copy link
Contributor

Is it planned to add more devices?
Maybe have 5 devices 'Desktop', 'Tablet Landscape', 'Tablet horizontal', 'Mobile Landscape', 'Mobile horizontal'.
So the new devices appear inside the first MenuGroup as a MenuItem.

@gziolo
Copy link
Member

gziolo commented Jan 20, 2022

Noting here that we are discussing a general strategy for making the editor interface extensible in #37448.

@draganescu draganescu removed their request for review January 31, 2022 10:13
simison added a commit that referenced this pull request May 12, 2023
First pass at continuing work on extending preview menu in editors.

The work is entirely copied from #36119, which includes work from #31984

Tony Arcangelini <tony@arcangelini.com>
Piotr Delawski <piotr.delawski@gmail.com>
@gziolo
Copy link
Member

gziolo commented Sep 3, 2024

Implemented with #64644. Thank you for your contribution.

@gziolo gziolo closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] In Progress Tracking issues with work in progress [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow external preview links in Preview dropdown to be extended
9 participants