Try using DataViewsPicker in an updated media modal #71944
Conversation
tellthemachines
added
[Type] Enhancement
|
Size Change: +72.3 kB (+3.3%) Total Size: 2.27 MB
ℹ️ View Unchanged
|
| import { useSelect } from '@wordpress/data'; | ||
| import { useCallback, useRef } from '@wordpress/element'; | ||
| import { useCallback, useRef, useState } from '@wordpress/element'; | ||
| // @ts-ignore |
There was a problem hiding this comment.
Should this be /* tslint:disable-next-line */? Sorry, that's all I've got tonight 😄
There was a problem hiding this comment.
Nice start, great to see progress on new media library work!
A couple of questions / thoughts:
- The featured image field is a natural one to try this out with while it's a proof of concept. At least in theory, it should be possible for consumers to replace the media library by using a filter rather than us needing to change the field directly.
I.e. in the editor package the existing modal is hooked up via:
addFilter(
'editor.MediaUpload',
'core/editor/components/media-upload',
() => MediaUpload
);
If we either update the modal to use a similar interface to MediaUpload (or add a wrapper that conforms to that interface) then this new modal could be used for the block editor and the featured image field without needing to change the field itself. It'd then be possible to toggle the new media library via an experiment as there'd be a single filter to switch on or off. Could something like that work?
Another idea for testing out the modal if you wanted to work on this iteratively before it's integrated to anything could be to add a Storybook entry where we can try it out separately. I recently tried this for the fields package over in #71864 and it seemed to work well for fields so far.
packages/media-utils/README.md
Outdated
|
|
||
| Undocumented declaration. | ||
|
|
||
| ### MediaUploadModal |
There was a problem hiding this comment.
While this is still experimental, should we export this as a private component (i.e. behind lock/unlock) rather than a public API? That way we can continue altering the params, etc, in subsequent PRs without having to worry about plugins possibly starting to use it.
The filter works on the |
Ah, good catch! That field should most likely be using the filtered one from the block editor, otherwise plugins using custom media libraries likely wouldn't be able to filter it. |
If you look at how AMP is modifying the media modal via that filter, I don't think back compat is going to be possible: Last time I tried changing the component passed to the filter it broke a lot of plugins (#70648). In the long-term, a deprecation of that filter might be the best option. I don't think any updated version of the media modal should be beholden to the filter, and it'll take a while to figure out the right way to provide extensibility for anything new. Separately I thought I'd mention that |
Oh great point, I'd forgotten about that. So what do you reckon in the shorter-term, would it be a matter of instances that want to use it simply importing the component directly as in this PR? Or could we add a Gutenberg experiment and conditionally export the "new" media library from within the block-editor package where |
How can we best deprecate the filter while ensuring that the editor gets the new modal by default? The only thing that occurs to me is using a wrapper component that checks for the existence of the In other news, the modal's looking a bit more presentable now, but for some reason the dataviews footer refuses to be sticky: Not sure what's going on there. |
I was thinking that we stop using it entirely (no calls to
Replace the I think this happens some time before the deprecation, the deprecation only happens when stabilizing the updated modal. If we also want to continue using filters for extensibility, then a new filter is implemented using a different name. |
Isn't this going to break things for people? It's not really a deprecation if we don't provide advance notice , just a breaking change 😅 (I wouldn't consider having the new modal behind an experiment flag to be advance notice, because experiments in Gutenberg have very low visibility) Not saying we shouldn't do it, but maybe we can support the filter for a little while to give folks the chance to move off it (and while we work out extensibility for the new modal). Unless the filter is currently broken already? |
Some way to allow users to continue using the old modal for a while is probably worth considering, almost like the way there are plugins for classic editor/widgets (though not saying this should be a plugin). Other than that, I'm not sure what other options there are. It's disruptive, but I don't think there's any other way to handle these types of changes. |
|
I'm not as concerned about users of the modal because it's mostly a UI redesign: the functionality remains the same. But for anyone who depends on the filter to add some custom workflow, removing the filter will break that. So I was thinking of some way to detect whether the filter is being used or not. |
tellthemachines
force-pushed
the
try/new-media-modal
branch
from
d75ead3 to
94fa8b9
Compare
We're talking about keeping it as an experiment first I think, so there's time to decide these things. For me, users using the modal are the people who will have their workflows broken, so it's the same thing. Detecting active filters could work, but we don't know what the filters do. It may still be that the new modal works better for a user, but some plugin is adding a filter and because of that the user has no option to switch. |
tellthemachines
force-pushed
the
try/new-media-modal
branch
2 times, most recently
from
79dc465 to
7d9a3f5
Compare
tellthemachines
force-pushed
the
try/new-media-modal
branch
from
7d9a3f5 to
c5e1a6e
Compare
|
It lives! Working pretty well. Pretty minor, but I noticed that the titles overflow doesn't behave coz the titles are 
Tried to fix it here: |
tellthemachines
force-pushed
the
try/new-media-modal
branch
from
c5e1a6e to
26a02db
Compare
| privateApis as blockEditorPrivateApis, | ||
| // @ts-ignore | ||
| privateApis as blockEditorPrivateApis, | ||
| } from '@wordpress/block-editor'; |
There was a problem hiding this comment.
This line was erroring and preventing my commit, not sure why 🤷
There was a problem hiding this comment.
I saw there's an issue here tracking the problem now - #72560.
|
I've updated the PR to add an experiment flag, and replace the block editor MediaUpload component with this one when the experiment is enabled, which allows us to test it better. This has surfaced a bug with the Dropdown/Modal nesting. When clicking outside the dataviews filters in the modal, the whole thing closes: modal_closing.mp4I traced it back to the Dropdown component closing due to an onBlur event targeting the Menu component (which dataviews filters use). I'm not sure why that happens, but it can be fixed by ensuring that Dropdown only listens to its own events. It's a small fix so I'll put up a separate PR for it. |
tellthemachines
requested review from
ellatrix and
kevin940726
as code owners
|
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. |
| } | ||
|
|
||
| // Use onUpload if provided, otherwise fall back to uploadMedia | ||
| const handleUpload = onUpload || uploadMedia; |
There was a problem hiding this comment.
This is a workaround for featured image, because mediaUpload isn't available in the block editor settings when that component loads. It's not really working properly though, because uploadMedia uses apiFetch directly instead of receiveEntityRecords so the modal items don't refresh 😞
The mediaUpload component the other instances are using lives in the editor package. I'm not sure if it would be worth moving/duplicating or something like that.
|
For reviewers: check the list of "known issues" in the description if you come across any breakage, I'm updating it as I go 😅 |
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
This is working well. I think it's good enough to ship for the experiment, with any issues fixable in other PRs. Most of the code comments I left are very minor and I think things will change as the code is iterated on anyway.
There are a few places I noticed the media modal doesn't work:
- Document Sidebar > Set Feature Image - still shows the old modal
- Inserter > 'Media' tab > Open Media Library - opens the new modal, but trying to insert something results in a missing 'core/attachment' block being inserted, probably something related to the records being slightly the wrong format.
- There was another place (I think it was a block) that allows opening the media modal in the sidebar, but I've forgotten where now (sorry!)
| * @param {Function} root0.render Render prop function that receives { open } object. | ||
| * @return {JSX.Element} The component. | ||
| */ | ||
| function ConditionalMediaUpload( { render, ...props } ) { |
There was a problem hiding this comment.
Minor: I know it's a temporary component for the experiment, but maybe it's worth only having one of these components in the block editor package that's imported where needed within the package.
I was wondering if the logic could be in the existing MediaUpload, though I guess that means anyone importing the component gets new modal. Maybe there are also other concerns with that.
There was a problem hiding this comment.
Yeah, I was just going to comment with a similar idea. Could we put ConditionalMediaUpload in components/media-upload-modal to remove a little duplication? Not a huge issue, just a nit 😄
| * | ||
| * @return {Component} The component to be rendered. | ||
| */ | ||
| const MediaUploadModal = () => null; |
There was a problem hiding this comment.
Very minor as well, but I wonder if it could be called something like DataViewsMediaModal to distinguish it a bit more from the other component.
| privateApis as blockEditorPrivateApis, | ||
| // @ts-ignore | ||
| privateApis as blockEditorPrivateApis, | ||
| } from '@wordpress/block-editor'; |
There was a problem hiding this comment.
I saw there's an issue here tracking the problem now - #72560.
| // Transform the records to the expected format | ||
| const mediaItems = useMemo( () => { | ||
| return ( mediaRecords || [] ).map( ( item: any ) => | ||
| transformAttachment( item ) |
There was a problem hiding this comment.
So at the moment the media items passed into DataViewsPicker are being transformed? I'm not sure about this choice, I think DataViewsPicker should work with something closer to REST API / core data style records. That way it's more likely to be compatible with fields and other shared DataViews functionality. Maybe I'm not understanding the permutations though.
transformAttachment could still be used for the selected items passed to the callback to make sure it's compatible with the expected results of the old media modal.
There was a problem hiding this comment.
We're retrieving the attachment post type which doesn't have a url property, which all the places that use it seem to expect.
I guess we could try only transforming it on selection.
| () => [ | ||
| { | ||
| id: 'select', | ||
| label: multiple ? __( 'Select' ) : __( 'Select' ), |
There was a problem hiding this comment.
Nit: It's the same string 😅
| id: 'url', | ||
| type: 'media' as const, | ||
| label: __( 'Media' ), | ||
| render: ( { item }: { item: Attachment } ) => ( |
There was a problem hiding this comment.
I think the poster isn't working for audio tracks and videos:
Here's the old media library:
Are there media fields that we can use (from the fields package?) or is this something we'll need to start implementing in a follow-up?
|
|
||
| const defaultLayouts = useMemo( | ||
| () => ( { | ||
| [ LAYOUT_PICKER_GRID ]: {}, |
There was a problem hiding this comment.
As this never changes, it could be defined in a const outside of the component instead of in a useMemo
|
Not a blocker as this is being proposed behind an experiment, but something I'm wondering about is how this modal might work if/when we have media-specific fields for it to use that might exist in the Then it'd be up to each button that triggers the modal to dispatch an action to open (and possibly configure) that single instance of the modal, rather than output the modal itself. Again not a blocker, just wondering how the fields package and modal might interact, but this could be pursued or explored in a follow-up (and perhaps any media fields should be defined alongside a modal?). This already provides a great foundation for those sorts of explorations IMO! |
I'm not sure I understand. Other media-specific fields would use it just like the featured image field does in this PR, or am I missing something? |
Oh, sorry, my statement was ambiguous! What I'm imagining is if/when we have some generic fields that are designed to be used in DataViews instances or the Picker for selecting or viewing media items. And where those fields will live vs where the modal lives. Fields like:
Eventually we might have a suite of those sorts of fields that will need to live somewhere, and this modal might conceivably use those fields within the modal. However if those fields live in the same package as
It isn't a blocker for now as we don't (yet) have these kinds of dedicated fields specifically for the attachment post type (they're inlined in the modal itself as you've done here). But it's an area that I could imagine we might need if/when we have a DataViews-based media library as well as this modal, so that they can share common fields. |
|
One idea for those fields, would be for them to live in a separate package, i.e. a |
|
Hmm yeah it's probably unsustainable to lump all the fields in their own package. The editor package also has at least one field defined in it. It might be that fields end up living closer to the places they're used, instead of in their own package. I don't really have opinions on that right now 😄 |
No worries! I think it'll become clearer as we try out some of these ideas... let's continue the discussion on a subsequent WIP PR 😊 |
But would the featured image upload field then also be a media field? 😅 I guess it's a post field really. I also think the featured image upload field should be changed to a general 'MediaUpload' field at some point. We should capture these things in issues. |
💯 Does what it says on the tin. Could we merge an iterate? Kapture.2025-10-24.at.12.55.26.mp4Just checking, the settings cog dropdown in the media modal allows field selection, but since it's a grid view it doesn't do anything. Is that a dataviews thing? I mean to say, something we could tell dataviews not to display? 
|
We can, I just can't stop tinkering with it 😅 Thanks for the reviews everyone! |
Sorry, I didn't mean to be pushy 😄 I think it's a great step forward! 🎉 |
Co-authored-by: tellthemachines <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: talldan <[email protected]>
What?
Try using DataViewsPicker and the Modal component for an updated media modal that can be used anywhere we need to select media.
I've added an experiment flag for the feature, and a new
editor.mediaUploadModalso that the currenteditor.mediaUploadcan remain undisturbed, due to the unfortunate fact that passing a functional component to that filter causes plugin breakage (see #70648).This now replaces all instances of the media modal. The featured image field one remains unfiltered: because it isn't part of the editor itself, but lives in wp-admin, the filter shouldn't be necessary. (the official purpose of the filter is to enable non-WP block editor instances to use their own media library implementations)
Known issues
mediaTypeis set incorrectlyTesting Instructions