Media Editor Experiment: Add a route, based on the media editor modal, refactor the modal components#77994
Merged
Conversation
|
Size Change: +221 B (0%) Total Size: 7.92 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a route-based Media Editor experience (so it can be opened via a direct URL and rendered outside a modal) and refactors the existing @wordpress/media-editor modal implementation by extracting a reusable MediaEditor component that can be framed by either a modal or a page.
Changes:
- Added a new
/media-editor/$idroute package and stage that renders the editor inside an@wordpress/admin-uiPage. - Refactored
MediaEditorModalto delegate to a new reusableMediaEditorcomponent via arenderFrameprop. - Added experiment-gated wp-admin bootstrap wiring for a hidden deep-link admin page.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| routes/media-editor/style.scss | Route-specific layout container styles and imports of media-editor built styles. |
| routes/media-editor/stage.tsx | New route stage rendering MediaEditor inside an admin-ui Page frame with breadcrumbs/back behavior. |
| routes/media-editor/route.ts | Route config with beforeLoad validation and dynamic title resolution. |
| routes/media-editor/package.json | Declares the new route package and its page targets/dependencies. |
| packages/media-editor/src/style.scss | Includes the new MediaEditor component SCSS in the package build. |
| packages/media-editor/src/private-apis.ts | Exposes MediaEditor via private APIs for consumers like routes. |
| packages/media-editor/src/components/media-editor/style.scss | New shared styling for the extracted MediaEditor layout (canvas/sidebar/skeleton/snackbar). |
| packages/media-editor/src/components/media-editor/index.tsx | New extracted MediaEditor implementation: save flow, keyboard handling, confirm discard dialog, and frame abstraction. |
| packages/media-editor/src/components/media-editor-modal/style.scss | Simplifies modal-specific CSS now that layout styles live in MediaEditor. |
| packages/media-editor/src/components/media-editor-modal/index.tsx | Refactors modal wrapper to render MediaEditor with a modal renderFrame. |
| package.json | Adds the media-editor wpPlugin page entry. |
| package-lock.json | Adds workspace linkage for the new @wordpress/media-editor-route package. |
| lib/load.php | Loads the media editor experimental bootstrap only when the experiment is enabled. |
| lib/experimental/media-editor/load.php | Registers the hidden wp-admin page entry point for deep links into the route-based editor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Flaky tests detected in c9f1be6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/25539671518
|
ce9b310 to
3eea184
Compare
This was referenced May 7, 2026
andrewserong
commented
May 7, 2026
Comment on lines
+97
to
+108
| export interface MediaEditorProps { | ||
| fields?: Field< Media >[]; | ||
| id: number; | ||
| media: Media | null; | ||
| hasEdits: boolean; | ||
| aspectRatioPresets?: AspectRatioPreset[]; | ||
| onClose?: () => void; | ||
| onSaved?: ( result: MediaEditorSaveResult ) => void; | ||
| renderFrame: ( props: MediaEditorFrameProps ) => JSX.Element; | ||
| noticesClassName?: string; | ||
| noticesPortalElement?: Element | null; | ||
| showCloseButton?: boolean; |
Contributor
Author
There was a problem hiding this comment.
Very happy for feedback on the props for this component: it can be tricky to not let it become just a grab bag of things!
Member
There was a problem hiding this comment.
For a private, early-stage component I think it's okay for now. If you're worried about separating concerns, maybe have a frame group:
frame: {
render: ( props: MediaEditorFrameProps ) => JSX.Element;
showCloseButton?: boolean;
shouldCloseOnEsc?: boolean;
...whateverElseBelongsHere
}
Member
There was a problem hiding this comment.
Oh, a couple of other things:
- is
fieldsclear enough? Or should we be explicitmetadataFields - remind me why we need
mediaandhasEdits? Isn't this handled internally via the core store?
Contributor
Author
There was a problem hiding this comment.
remind me why we need media and hasEdits? Isn't this handled internally via the core store?
Good catch! I've moved them internally 👍
is fields clear enough? Or should we be explicit metadataFields
I went with fields for consistency with DataViews and DataViewsPicker that use that as a prop. We could rename, but metadataFields makes me think of image metadata that gets baked into the image itself 🤔
For a private, early-stage component I think it's okay for now. If you're worried about separating concerns, maybe have a frame group:
Thanks! I think I prefer flat props as I find them slightly easier to deal with.
I might otherwise leave this props here for now and we can always tweak in follow-ups. Unless you feel strongly about any of these options, of course!
andrewserong
commented
May 7, 2026
Comment on lines
+86
to
+95
| export interface MediaEditorFrameProps { | ||
| children: ReactNode; | ||
| headerActions: ReactNode; | ||
| onRequestClose: () => void; | ||
| onKeyDown: ( event: ReactKeyboardEvent< HTMLElement > ) => void; | ||
| shouldCloseOnClickOutside: boolean; | ||
| isSaving: boolean; | ||
| hasChanges: boolean; | ||
| hasMedia: boolean; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
And likewise for the renderFrame, it's each for this to be a bit random. How does it look?
|
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. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…e description of the experiment in Gutenberg
3eea184 to
716ada7
Compare
ramonjd
approved these changes
May 8, 2026
Member
ramonjd
left a comment
There was a problem hiding this comment.
I just wrote nitpicky things. This is a great start on being able to share the modal around.
| onClose?.(); | ||
| }; | ||
|
|
||
| const handleRequestClose = () => { |
Member
There was a problem hiding this comment.
Not for here, but I was wonder how we'd handle closing via navigation? So if I click to a new page (assuming the admin or other nav bar is there)
Contributor
Author
There was a problem hiding this comment.
Good catch. Yeah, I think let's look into the navigation guard in follow-ups 👍
Comment on lines
+97
to
+108
| export interface MediaEditorProps { | ||
| fields?: Field< Media >[]; | ||
| id: number; | ||
| media: Media | null; | ||
| hasEdits: boolean; | ||
| aspectRatioPresets?: AspectRatioPreset[]; | ||
| onClose?: () => void; | ||
| onSaved?: ( result: MediaEditorSaveResult ) => void; | ||
| renderFrame: ( props: MediaEditorFrameProps ) => JSX.Element; | ||
| noticesClassName?: string; | ||
| noticesPortalElement?: Element | null; | ||
| showCloseButton?: boolean; |
Member
There was a problem hiding this comment.
For a private, early-stage component I think it's okay for now. If you're worried about separating concerns, maybe have a frame group:
frame: {
render: ( props: MediaEditorFrameProps ) => JSX.Element;
showCloseButton?: boolean;
shouldCloseOnEsc?: boolean;
...whateverElseBelongsHere
}
Comment on lines
+97
to
+108
| export interface MediaEditorProps { | ||
| fields?: Field< Media >[]; | ||
| id: number; | ||
| media: Media | null; | ||
| hasEdits: boolean; | ||
| aspectRatioPresets?: AspectRatioPreset[]; | ||
| onClose?: () => void; | ||
| onSaved?: ( result: MediaEditorSaveResult ) => void; | ||
| renderFrame: ( props: MediaEditorFrameProps ) => JSX.Element; | ||
| noticesClassName?: string; | ||
| noticesPortalElement?: Element | null; | ||
| showCloseButton?: boolean; |
Member
There was a problem hiding this comment.
Oh, a couple of other things:
- is
fieldsclear enough? Or should we be explicitmetadataFields - remind me why we need
mediaandhasEdits? Isn't this handled internally via the core store?
| className="media-editor-route" | ||
| ariaLabel={ title } | ||
| breadcrumbs={ | ||
| <Breadcrumbs |
Member
There was a problem hiding this comment.
Does clicking in the breadcrumb items navigate away immediately? Just wondering whether we need to consider the editor’s close guard.
Contributor
Author
There was a problem hiding this comment.
Oh, good point! I started to look into it after your comment, but it sounds slightly rabbit-holey (at least for a Friday). I think I'll make a note for it for follow-ups rather than dealing with it in this PR.
Contributor
Author
|
Thanks again for the review! There's a flaky e2e test. Out of an abundance of caution I'll leave this PR here for now, and will merge it on Monday morning (possibly after a rebase). Cheers! |
This file contains hidden or 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
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.
What?
Part of:
This PR proposes adding a route-based version of the media editor modal experiment, so that the new media editor could be accessed from a direct URL (still behind an experiment of course), and rendered outside of a modal context.
Why?
However, the main reason why to do this at this stage is actually the refactor of the media editor modal components in the
media-editorpackage. While there's quite a few files changed here, I think overall it's not too complex a change. At least I hope it isn't!My goal is to help get the shape of the files and APIs in
media-editorinto something of a stable shape for use in any situation that might call for the media editor components.How?
MediaEditorModalcomponent with a more generalMediaEditorcomponentMediaEditorcomponent takes arenderFrameprop for rendering the frame of the editor. This could be a modal or it could be a page/routesNote that there is no visible entry path to this new route. Hooking it up to other places is a concern for later PRs.
Testing Instructions
Screenshots or screencast
When testing, with all the experiments active, you should no longer see an "Edit Media" button in the image block, as we're using the Crop button for the media editor modal:
I've also tried to better clarify this experiment in contrast to the Modal experiment. Here's how they look on the experiments page:
Use of AI Tools
Codex