-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Editor: Move media upload from utils #7978
Conversation
ec234a1
to
4bbfbce
Compare
4bbfbce
to
cc763bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this one a rebase to resolve merge conflicts. There were a few updates from #7566 which needed to be ported to the new editor-media-upload/media-upload.js
file.
I found it a bit difficult to test various workflows, as I encountered several unrelated bugs (#8022, #8023, #8025, #8026).
For workflows which do work on master, they also work here as well.
@@ -81,4 +81,10 @@ export const EDITOR_SETTINGS_DEFAULTS = { | |||
|
|||
// Allowed block types for the editor, defaulting to true (all supported). | |||
allowedBlockTypes: true, | |||
|
|||
// Maximum upload size in bytes allowed for the site. | |||
maxUploadFileSize: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that long-term we want these to be considered editor settings, since they apply to the site as a whole. Seems like something which should ideally live in @wordpress/core-data
, though I do not believe they are yet available from the REST API.
docs/reference/deprecated.md
Outdated
@@ -14,6 +14,9 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo | |||
- `wp.utils.decodeEntities` has been removed. Please use `wp.htmlEntities.decodeEntities` instead. | |||
- All references to a block's `uid` have been replaced with equivalent props and selectors for `clientId`. | |||
- The `MediaPlaceholder` component `onSelectUrl` prop has been renamed to `onSelectURL`. | |||
- `wp.utils.getMimeTypesArray` has been removed. | |||
- `wp.utils.mediaUpload` has been removed. Please use `wp.editor.editorMediaUpload` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first thought: Is this something which should be exposed on a public API? Then seeing we're using this in blocks, I'm left wondering: What does this have to do with the editor? It's just media uploads? Definitely seems like something for core-data
.
Thinking more about naming / appropriateness in editor:
|
4cc3450
to
17cd99c
Compare
Agreed, I updated PR to use shorter version.
It has been around for quite some time. We use with many core blocks. |
It was more about the earlier point of whether this belongs as an editor function, vs. a core-data action. Prefixing it would serve the immediate need of requiring it in our current blocks. |
allowedType, | ||
filesList, | ||
maxUploadFileSize, | ||
onError = noop, | ||
onFileChange, | ||
} ) { | ||
const postId = select( 'core/editor' ).getCurrentPostId(); | ||
const { | ||
getCurrentPostId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in retrospect, I think this is the one critical piece where having this be an editor-specific function could make sense, in effecting that the media be attached to the post being edited.
That said, with recent refactoring for store registries (#7527), exposing this as a simple function on the editor global does not seem tenable. If we want it, it seems like something which would be a higher-order component to inject the uploadMedia
prop into the underlying component to take advantage of the contextualized store.
I think we should punt this to 3.4, so that we can offer an alternative solution, than to just remove the functionality or update it to a non-ideal alternative. |
Do you think it's possible to land this as is and create an issue to come up with a better alternative? This is blocking the |
@youknowriad My main concern would be the lack of an alternative to give in the release. I'd be fine with merging it immediately after, if that works? |
If we agree that initial implementation of HOC would only pass down mediaUpload function as prop, I can refactor it on Monday. It would be called withMediaUpload and located in editor package. |
yep, let's at least wait for the release and then we can merge (before or after refactoring) |
There is one usage which makes it difficult to convert to HOC: In this case, it's used inside the transform, so it doesn't have access to props at all because it is part of block settings. I'm trying to find a solution which would allow us to move it to the lifecycle of the |
I personally don't think Let's move forward with just the |
a3795d0 shows that this is a more complex task which has some implications on the way we handle transforms. I will remove this commit, bump deprecations to be scheduled for 3.6 release and merge. Let's continue the discussion and agree on the further steps. |
a3795d0
to
8cbaf6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -1093,6 +1092,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) { | |||
// https://github.com/WordPress/gutenberg/issues/5667. | |||
add_filter( 'user_can_richedit', '__return_true' ); | |||
|
|||
wp_enqueue_script( 'wp-utils' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have commented to explain the context for this line. I'm in the course of removing the deprecated ./utils
and it wasn't clear to me what relation this has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, when removing folder, you probably can remove all wp-utils occurrences in this file.
Description
Part of #3955.
This is the last PR which will allow us to fully deprecate
wp.utils
module. We will have to keep a copy of deprecated methods for the next two releases to give some time plugin authors to migrate their code.This PR schedules for removal 3 functions from
wp.utils.*
namespace:wp.utils.getMimeTypesArray
has been removed.wp.utils.mediaUpload
has been removed. Please usewp.editor.editorMediaUpload
instead.wp.utils.preloadImage
has been removed.That means
mediaUpload
will become an internal implementation ofeditorMediaUpload
function.In addition to that, we get rid of the global variable
window._wpMediaSettings
in favor of using editor settings, similar to what we do for other similar config values.Testing
Make sure you can still use all deprecated methods in
wp.utils.*
namespace and they warn on the JS console about the upcoming deprecation.Make sure you can upload all type of files using blocks like Audio, Video or Image.