Add extensibility to PreviewOptions #50605
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simison
added
[Status] In Progress
|
Size Change: +910 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5efcca7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5813250847
|
mtias
reviewed
May 20, 2023
mtias
reviewed
May 20, 2023
|
Can we split this in two? I think part of the reason it has stalled is that extending the menu is pretty simple, but extending the canvas needs more thought. It'd be good to introduce the first separately. |
simison
commented
May 25, 2023
3 tasks
simison
force-pushed
the
add/preview-extensibility
branch
from
473fbf0
to
37bb65d
Compare
@mtias agreed, and I think since previews could be done in various ways, we should also approach expanding the API step-by-step. I've updated the code to be very minimal and restricted, yet still quite powerful. I also elaborated on future ways to expand the API with screenshots. Ready for review! |
simison
force-pushed
the
add/preview-extensibility
branch
from
37bb65d
to
5efcca7
Compare
simison
added
the
[Type] New API
youknowriad
reviewed
Aug 10, 2023
| @@ -51,6 +51,7 @@ | |||
| "@wordpress/html-entities": "file:../html-entities", | |||
| "@wordpress/i18n": "file:../i18n", | |||
| "@wordpress/icons": "file:../icons", | |||
| "@wordpress/interface": "file:../interface", | |||
There was a problem hiding this comment.
I don't think the block editor should depend on "interface", interface is a package to build wp-admin pages (post editor, site editor...) and block editor is a reusable package that can be seen as "TinyMCE" for block editors.
| @@ -79,6 +80,9 @@ export default function PreviewOptions( { | |||
| { __( 'Mobile' ) } | |||
| </MenuItem> | |||
| </MenuGroup> | |||
| <PluginPreviewMenu.Slot> | |||
| { ( fills ) => <MenuGroup>{ fills }</MenuGroup> } | |||
| </PluginPreviewMenu.Slot> | |||
There was a problem hiding this comment.
I'm guessing the reason you depend on interface is because the "PreviewOptions" is placed within the block editor. That was a mistake IMO, I've tried to fix it a long time ago (sometimes duplicating code is better than sharing code) #30906 but I closed that PR because the added value for that refactoring was a bit small. Potentially today, we could revive that PR (aka remove the shared PreviewOptions component and move it to edit-site and edit-post instead)
| const PluginPreviewMenu = ( { name, onClick, title, ...props } ) => ( | ||
| <Fill> | ||
| <MenuItem | ||
| className="block-editor-post-preview__button-resize" |
There was a problem hiding this comment.
This classname doesn't follow our guidelines, the component is within "interface" (which can be acceptable) but the classname is like a "block editor" one.
|
I really appreciate the examples and the thoughts in the descriptions, it all sounds reasonable, the main problem now is the dependency between block-editor and interface. |
|
Thanks for the review @youknowriad ! Happy to continue work on this. Did I understand you right; it would be better if I moved the component to It's been a while so I don't remember my thinking around |
|
I guess IMO, the ideal would be to move the "PreviewOptions" to |
This was referenced Mar 26, 2024
|
@youknowriad @mtias Now that the editor wrapper has been refactored in Gutenberg (which Riad mentioned should happen prior to this API addition), it's a good time to resume allowing the preview dropdown to be extended. Let me know if the premise of this PR needs updating or if the timing isn't great from the Gutenberg team's perspective. Otherwise, we'll proceed with @pkuliga to draft a proposal for:
So basically minimal, less flexible way to get started and see how that'll get adopted first. I elaborated in the PR description earlier on the do/don'ts of the API and its potential future direction. |
|
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 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. |
|
Sounds good to me. I see a few developer related APIs planned for WP 6.7, this would be a good addition as well IMO. |
|
Yes, let's keep this minimal while we learn how it's used. |
|
@youknowriad @mtias we can continue in #64644 |
24 tasks
|
closing in favor of the other PR |
|
Implemented with #64644. |
|
Although looking at the code, I figured out that this one was more complex as it also tried to integrate more deeply into the editor screen by allowing to replace the visual code editor. |
|
@gziolo there are two main aspects:
There's plenty to figure out on both across different block editors, so it made sense to focus on just dropdown in #64644, and have first iteration pretty limited. I've elaborated in PR description (see "Future iterations") potential future directions for the API. |
Do we have an issue where we track the future iterations? "Extending the preview canvas" sounds like something that would want to discuss next. Although, that was never considered as an extensibility point despite the fact that there is a way for switching from More Menu between the visual and code editor. |
|
Moved follow-ups to an issue: (edit, correct link) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #25309
Builds from #36119, which includes work from #31984.
What?
Most basic version of extending plugin preview menu; and only that.
Does not allow replacing editor canvas with custom previews yet.
The extension point also does not allow just any custom HTML within the dropdown. Instead the extension is declarative by nature, restricting to specific props only (background). This gives us freedom to change the UI later, and plugin developer wouldn't need to care about menu's internal structure.
Example usage:
Example result:
Single item example
API allows:
onClickhandler to open a modal for example.API does not allow:
Future iterations
Allow new tabs
Example API could just require adding
href="#"and we would handle it as new tab automatically:Resulting:
Allow selecting items
Allowing selecting (or "setting device") would work well with conjuction in allowing custom canvas preview, or custom canvas preview sizes. #36119 has one technical implementation idea.
Example API could look like:
Resulting:
Meanwhile current API format would continue supporting e.g. popups:
Allow new groupings
Example API could look like:
Resulting:
Multiple groupings combined with selection could allow something more sophisticated such as:
Why?
See issue #25309
Extending the preview menu allows variety of publishing flows and tools, some examples:
An example of more complex preview capabilities from Substack:
Screen.Recording.2023-07-26.at.12.03.19.mov
How?
Extension point utilizing slot/fill.
I didn't add tests yet; I will add those once we are certain this API is acceptable first step.
Testing Instructions
Test with:
Testing Instructions for Keyboard