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

Allow the Preview Menu to be extendible by plugins #25430

Closed
wants to merge 4 commits into from

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Sep 17, 2020

Description

  • Adds two components that can be used by plugins to register new items in the "Preview" menu
    • <PluginPreviewMenuItem> creates new menu entries
    • <PluginPreview> registers what will render in the main area when that entry is selected

Screenshots

2020-09-17_18-12

Concerns

  • Overall approach + organization. I'm new to the Gutenberg codebase.
  • React Native, haven't thought about it at all
  • Will a plugin using <PluginPreview> be able to access data stores with useSelect to get the info it needs? Or will that info need to be passed down by props?

How has this been tested?

Creating this test plugin and using wp-env to run it:

import React from "react";

(function (wp) {
  var registerPlugin = wp.plugins.registerPlugin;
  //var el = wp.element.createElement;
  const { Fragment } = wp.element;
  const { PluginPreviewMenuItem, PluginPreview } = wp.blockEditor;

  const PluginPreviewTest = () => (
    <Fragment>
      <PluginPreviewMenuItem previewId="preview-custom-1">
        Custom Preview 1 Menu Text
      </PluginPreviewMenuItem>
      <PluginPreview previewId="preview-custom-1">
        <div>
          <h4>Custom Preview 1 Content</h4>
        </div>
      </PluginPreview>
    </Fragment>
  );

  registerPlugin("plugin-preview-test", {
    render: PluginPreviewTest,
  });
})(window.wp);

Test plugin repository with webpack build included here.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@simison
Copy link
Member

simison commented Sep 18, 2020

Issue: #25309

cc @westonruter


Each of these takes 2 props, `deviceName`, and `children`.

- `deviceName` - A string that serves as an internal identifier for your
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd keep this more generic and not assume it's a device.

Think e.g. AMP previews, social media previews, etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the external interface to use previewId as a way to match the menu item with the content.

@mreishus mreishus force-pushed the add/preview-menu-slot-3 branch from 6690ad1 to ff0d1e2 Compare September 18, 2020 15:29
@mreishus
Copy link
Contributor Author

mreishus commented Sep 18, 2020

I'm not completely happy with:

  • The core/edit-post store is still storing which preview is selected in a variable named deviceType. Some previews will custom registered by plugins and not devices: (Example, social preview). I'm a bit wary of renaming this everywhere without guidance, as it could become a source of conflicts if this becomes a long-running branch.
  • coreDeviceTypes: Used to determine if a deviceType should be handled by the core <VisualEditor> or handed off to the plugin. I kicked around the idea of making an abstraction with functions listed below, but it seemed to make the code more difficult to understand.
    • isDeviceTypeCustom()
    • previewIdToDeviceType()
    • deviceTypeToPreviewId()
    • previewIdToSlotName()
  • The <VisualEditorOrPluginPreview> component living in edit-post/ and "knowing" about coreDeviceTypes and the magic slot name {'core/block-editor/plugin-preview/' + deviceType}, which both come from block-editor/

@mreishus mreishus force-pushed the add/preview-menu-slot-3 branch from 37df129 to f82031b Compare September 18, 2020 20:01
@mreishus mreishus marked this pull request as ready for review September 18, 2020 20:02
@mreishus mreishus changed the title Allow the Preview Menu to be extendible by plugins: WIP Allow the Preview Menu to be extendible by plugins Sep 18, 2020
@jasmussen
Copy link
Contributor

High level, this seems cool with me, and brings value to the preview dropdown. I'd love to see anything from AMP previews, to search result previews, to "behind paywall" previews in that dropdown, or whatever else developers might throw at it.

With regards to the technical implementation, I'll defer to Riad and Mikael.

@youknowriad
Copy link
Contributor

Is this something we'd want in the full site editor as well, how does it map to the "complementary areas" abstraction. Do you have any thoughts on that @jorgefilipecosta

@mreishus
Copy link
Contributor Author

mreishus commented Sep 21, 2020

My first try at this used Complementary Areas. (@youknowriad @jorgefilipecosta ) Here are the problems I found with it:

  • It started rendering Sidebar related components into the main area without me specifying it, which made me think the abstraction was built for working in the sidebar only. (Hence I wrote "wrong abstraction" in my commit message, apologies if it isn't)
    2020-09-17_14-59
    Here, I render <div>Content of the ABC3.4</div> into the main area. It was surrounded with a sidebar.
    Code used: https://github.com/mreishus/gutenberg/pull/1/files
  • I need to integrate with the current switching behavior between preview modes: "Mobile, Tablet, Desktop, (custom items here)." There is a check mark that shows which one is selected that behaves like a radio button (only one can be selected). I didn't see a way to integrate the three already existing items into a complementary area like this.
    2020-09-17_18-12

@mreishus mreishus force-pushed the add/preview-menu-slot-3 branch from f82031b to fcc3670 Compare September 21, 2020 14:06
@pierlon
Copy link
Contributor

pierlon commented Oct 8, 2020

I'm a developer working on the AMP plugin and this PR would be very helpful for us by getting rid of some of the hacks we have in place for rendering the AMP button in the editor post header settings. In our case however all we would want is to extend the preview menu by rendering our component within the PluginPreviewMenuItem slot, without the necessity of specifying a previewId and PluginPreview (which I would like to think would also be the use case for most developers).

@mtias mtias added the [Feature] Extensibility The ability to extend blocks or the editing experience label Oct 9, 2020
const { deviceType } = useSelect(
( select ) => ( {
deviceType: select(
'core/edit-post'
Copy link
Member

Choose a reason for hiding this comment

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

The block editor is a generic package that may be used outside WordPress, while the core/edit-post is a package to edit regular WordPress posts. I think we should avoid having the block-editor depending or querying data from core/edit-post.

I guess things will also not work as expected for edit-site scree where we don't have a core/edit-post module?

The device type is passed to the PreviewOptions as a prop. Could we rely on that prop instead?

Not directly related to this PR, but I think it makes sense to move the preview functionality under the WordPress/interface package. It is common to edit-post and edit-site. It has functionality like the singleEnableItems that can be used to store which preview is active. I guess the slot / fill here could also be abstracted as action item packages/interface/src/components/action-item/README.md.

Regarding the API, we are exposing PluginPreview, PluginPreviewMenuItem on the @wordpress/block-editor' package. As far as I know, the bock-editor does not expose slot & fills besides the ones used by blocks. All the plugin related API's are exposed under edit-post/edit-site. I guess for this case it may make sense too to make PluginPreview, PluginPreviewMenuItem under @wordpress/edit-site/edit-post. I guess we could implement the slot fills under WordPress/interface, and then on edit-site and edit-post, we would simply have wrappers that pass a specific scope. Similar to what we do for the sidebar and menu item slots.
The PreviewOptions component would have a prop for additional preview options that edit-site and edit-post use.

cc: @gziolo and @youknowriad In case you have a different option.

@gziolo
Copy link
Member

gziolo commented Oct 27, 2020

One thing that is a bit confusing in the whole Preview Menu is that: Desktop, Tablet and Mobile use the same component but apply some modifications to CSS. In that, it isn't really a preview but the Visual mode of the editor. In effect, if you change the mode to Code then these preview items don't work at all:

Screen Shot 2020-10-27 at 18 51 23

It's something that would need to be clarified first with designers to remove some confusion. Regarding the proposal, the whole idea makes sense to me. There some open questions like whether editor modes should be also extensible. What is the main factor that decides where to put new menu items? Should there be only one slot that renders the visual editor or code editor or something else registered by a plugin? It might be the most efficient to optimize usage by introducing a single component <PluginEditorContent type="preview" /> that would take care internally of registering a menu item in the corresponding menu group depending on the type defined: mode vs preview. Unless we want to consolidate that under one menu.

@jasmussen
Copy link
Contributor

One thing that is a bit confusing in the whole Preview Menu is that: Desktop, Tablet and Mobile use the same component but apply some modifications to CSS.

Two things:

  • The preview is increasingly accurate as themes style the editor. With the lighter dom, the reduced UI, and hopefully soon the iframe, the barrier to a 1:1 with the frontend is lower than ever.
  • The preview is meant to also serve the purpose of letting you provide basic responsive editing changes, such as outlined in Responsive previewing and device-specific editing #19909. That ticket needs to be revisited with some fresh thoughts, but the principle is the same, and the layout grid plugin already takes advantage of this.

In effect, if you change the mode to Code then these preview items don't work at all

We should gray those responsive preview menu items in that case.

@simison
Copy link
Member

simison commented Nov 11, 2020

Any specific action items to push this PR forward?

@simison
Copy link
Member

simison commented Feb 17, 2021

@pierlon hi! Would you have interest in helping this PR forward?

@mreishus
Copy link
Contributor Author

@jorgefilipecosta To move forward, the file packages/block-editor/src/components/preview-options/plugin-preview-menu-item/index.js needs to no longer depend on the package 'core/edit-post', is that correct? Do you have any suggestions on how to accomplish that?

@mreishus
Copy link
Contributor Author

BTW, I don't have much time available to work on this; if you see it and want to run with it, feel free.

@simison
Copy link
Member

simison commented Mar 15, 2021

Just FYI that the site editor (FSE) doesn't have the "preview in new tab" option because the whole editor is the preview:

image

@simison
Copy link
Member

simison commented Mar 15, 2021

Another aspect to think about:

E.g. AMP plugin adds preview button also in mobile screen size, while Gutenberg hides that preview on mobile:

image

There might be modes where it makes sense to keep showing the preview also on mobile.

@delawski delawski mentioned this pull request May 19, 2021
7 tasks
@delawski
Copy link
Contributor

The feedback left on this PR has been addressed in a new PR: #31984.

@gziolo
Copy link
Member

gziolo commented May 19, 2021

Does it mean that this PR should get closed in favor of #31984?

@delawski
Copy link
Contributor

Does it mean that this PR should get closed in favor of #31984?

@gziolo Yes, I believe so.

@gziolo
Copy link
Member

gziolo commented May 19, 2021

@mreishus, thank you for all the work done so far. Let's continue in the fresh PR that received some updates.

@gziolo gziolo closed this May 19, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants