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

Extract media upload logic part of @wordpress/editor to its own package #15521

Merged
merged 1 commit into from Jun 3, 2019

Conversation

@jorgefilipecosta
Copy link
Member

commented May 8, 2019

Description

This PR extracts the media upload function part of @wordpress/editor to its own package and uses it to implement file uploads in the widget screen.

To have the ability to select files from the media library we will need to expose a hook part of '@wordpress/edit-post' and that will be available in other PR as this one got big.

We should be able to add a gallery, images, audio, file blocks etc... And correctly upload a file by pressing the upload button or using drag & drop.
Uploading without the block being previously created (e.g: dragging images to the content) does not work yet because some blocks use mediaUpload from WordPress editor directly and will be refactored in separate PR's

How has this been tested?

I went to /wp-admin/admin.php?page=gutenberg-widgets.
I added image, gallery, file, audio blocks and I uploaded files to them using upload button and drag & drop without any problem.

...state,
...action.settings,
};
}

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 9, 2019

Contributor

Do we really need a store for this, if feels like for now we can just pass these media options as props to the block editors.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 9, 2019

Author Member

We are going to need an edit-post store anyway, so I took the chance to add the store here to make the PR that was adding the store smaller.
Provided we will have a store, adding the settings to the store makes this implementation consistent with what we do in other modules.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 9, 2019

Contributor

Provided we will have a store, adding the settings to the store makes this implementation consistent with what we do in other modules.

To be clear I'm not sure it's a good idea in the other modules (editor) too, it's just because we built that module first that it's there. I think it will simplify things a lot if we avoid it.

I'm not against introducing it later if we feel the need but I'm really unsure about it. It feels like we're just going to pass these settings down.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 9, 2019

Contributor

(And yea, we'll need a store that's true, it's more the settings in the store that bothers me)

@youknowriad
Copy link
Contributor

left a comment

So it seems like you moved the mediaUpload handler but not the MediaUpload filter which I guess is also needed and could live in this new package right?

Also, I'd have loved for this package to be just @wordpress/media and to avoid using wp.media. Actually, it could contain the current media scripts (cc @joemcgill @antpb) but I think it's a bit too early for this and this will be a project that require some time and investment.

That said, I'm not sure about the name of media-upload, maybe media-utils if it contains the filter as well?

@aduth any thoughts here?

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Also, I'd have loved for this package to be just @wordpress/media and to avoid using wp.media. Actually, it could contain the current media scripts (cc @joemcgill @antpb) but I think it's a bit too early for this and this will be a project that require some time and investment.

We need a way to access the current wp.media object, as widgets are expecting its existence, so I don't think we can use @wordpress/media.

That said, I'm not sure about the name of media-upload, maybe media-utils if it contains the filter as well?

My plan was to expose the filter in a small package, but on second thought probably a media-utils package is a better option.

@joemcgill

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Also, I'd have loved for this package to be just @wordpress/media and to avoid using wp.media. Actually, it could contain the current media scripts (cc @joemcgill @antpb) but I think it's a bit too early for this and this will be a project that require some time and investment.

One thing to keep in mind here is that the current wp.media is quite old now and is in need of quite a bit of work to decouple from old assumptions about how WordPress works, so any move to externalize this should be done with a strategy for replacing with a new application to manage media that can be integrated with the editor in a better way.

jorgefilipecosta added a commit that referenced this pull request May 24, 2019

Add blockEditor settings on the widget screen (#15788)
The block editor settings are required to ensure legacy widgets work as expected on the widgets block editor screen.

This commit is related to #15521 given that in both changes we add block editor settings support to the widget screen.

@jorgefilipecosta jorgefilipecosta force-pushed the add/media-upload-widgets-screen branch from cdd40c2 to 401ed2f May 24, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 24, 2019

This PR was rebased now that the settings mechanism was already merged as part of other PR. I also performed some updates namely now we have a media utils package that contains the util and a component that allows the usage of the media library.
With this PR in, we get full media support om the widget block editor.

@jorgefilipecosta jorgefilipecosta force-pushed the add/media-upload-widgets-screen branch 2 times, most recently from 6437ead to 69b8c47 May 27, 2019

@noisysocks

This comment has been minimized.

Copy link
Member

commented May 30, 2019

It's strange to me that we're using a filter for editor.MediaUpload when it is essentially a slot. Could this be a good opportunity to replace this filter with a Slot/Fill? cc. @gziolo

// packages/block-editor/src/components/media-upload/index.js

/**
 * WordPress dependencies
 */
import { createSlotFill, withFilters } from '@wordpress/components';
import { addAction } from '@wordpress/hooks';
import { deprecated } from '@wordpress/deprecated';

const { Slot, Fill } = createSlotFill( 'MediaUpload' );

function MediaUpload( props ) {
	return <Slot fillProps={ props } />;
}

MediaUpload.Fill = Fill;

addAction( 'hookAdded', 'core/block-editor/check-deprecated-media-upload', ( name ) => {
	if ( name === 'editor.MediaUpload' ) {
		deprecated( "addFilter( 'editor.MediaUpload', ... )", {
			alternative: '<MediaUpload.Fill>',
			plugin: 'Gutenberg',
		} );
	}
} );

export default withFilters( 'editor.MediaUpload', MediaUpload );
// packages/edit-post/src/components/layout/index.js

...
<MediaUpload.Fill>
	{ ( props ) => <MediaUploadImplementation { ...props } /> }
</MediaUpload.Fill>
...
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import { components } from '@wordpress/media-utils';

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

Could we have MediaUpload be a named export? This is more consistent with how e.g. @wordpress/components works.

import { MediaUpload } from '@wordpress/media-utils';

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

I'm proposing something similar in my comment in a different place in this PR :)

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 31, 2019

Member

Great minds think alike!

/**
* Internal dependencies
*/
import './components';

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

Why move this from components/index.js to hooks/index.js? It looks like the intention was that each of the edit-post hooks would be in their own file.


addFilter(
'editor.MediaUpload',
'wordpress/media-utils/replace-media-upload',

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

This filter is still in the @wordpress/edit-post package so I think that its identifier should reflect that. That is, the namespace should begin with core/edit-post instead of wordpress/media-utils.

@@ -13,6 +17,17 @@ import Layout from '../layout';
function EditWidgetsInitializer( { setupWidgetAreas, settings } ) {
useEffect( () => {
setupWidgetAreas();
addFilter(
'editor.MediaUpload',
'wordpress/media-utils/replace-media-upload',

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

This filter is in the @wordpress/edit-widgets package so I think that its identifier should reflect that. That is, the namespace should begin with core/edit-widgets instead of wordpress/media-utils.

/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import { utils as mediaUtils } from '@wordpress/media-utils';

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

Could we have mediaUpload be a named export? This is more consistent with how e.g. @wordpress/utils works.

import { mediaUpload } from '@wordpress/media-utils';

Could potentially also rename it to uploadMedia so that:

  1. It's less easy to confuse mediaUpload with MediaUpload.
  2. It follows our convention that function names start with a verb.
@@ -13,6 +17,17 @@ import Layout from '../layout';
function EditWidgetsInitializer( { setupWidgetAreas, settings } ) {
useEffect( () => {
setupWidgetAreas();
addFilter(

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

@wordpress/edit-post adds this filter on page initialisation but here in @wordpress/edit-widgets we're adding it on component initialisation. Should we be consistent?

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

We should pick one approach. It probably doesn't matter that much which one though. In terms of unit tests, this proposal might be even better.

} ) {
const settings = useMemo(

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

Is useMemo necessary here? Spreading the settings object to contain a new __experimentalMediaUpload property doesn't seem very expensive.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 31, 2019

Author Member

Yes, it is not very expensive computing the settings object itself. The reason I am using it is that without it each time WidgetArea renders settings would point to a different reference that would trigger a rerender BlockEditorProvider with new settings which then would need to update the block editor store with new settings and probably cause other rerenders.

if ( ! hasUploadPermissions ) {
return blockEditorSettings;
}
const mediaUploadBlockEditor = ( { onError, ...argumentsObject } ) => {

This comment has been minimized.

Copy link
@noisysocks

noisysocks May 30, 2019

Member

Having all of this logic in the component's render function is a little distracting. Maybe we could pull this function out into a new file which mirrors packages/editor/src/utils/media-upload/index.js?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 31, 2019

Author Member

I extracted the settings computation to a specific function. That makes the code easier to understand.

@jorgefilipecosta jorgefilipecosta changed the title Add file upload capabilities to the widgets screen Extract media upload login part of @wordpress/editor to its own package May 30, 2019

@jorgefilipecosta jorgefilipecosta added this to Genertic Block Editor Issues in Widgets screen May 30, 2019

@jorgefilipecosta jorgefilipecosta changed the title Extract media upload login part of @wordpress/editor to its own package Extract media upload logic part of @wordpress/editor to its own package May 30, 2019

* Internal dependencies
*/
import { mediaUpload } from './media-upload';
import { utils } from '@wordpress/media-utils';

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

Have you considered having:

Suggested change
import { utils } from '@wordpress/media-utils';
import { mediaUpload } from '@wordpress/media-utils';

or

Suggested change
import { utils } from '@wordpress/media-utils';
import { utils } from '@wordpress/media';
@@ -0,0 +1,35 @@
{
"name": "@wordpress/media-utils",

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

How about:

Suggested change
"name": "@wordpress/media-utils",
"name": "@wordpress/media-upload",

As of today, this package will contain only:

  • MediaUpload component
  • mediaUpload callback.

The description also highlights that the purpose of this package is strictly related to uploading functionalities.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta May 31, 2019

Author Member

Hi @gziolo the mediaUpload function was renamed to uploadMedia as @noisysocks suggested. Do you think it still makes sense to rename the package or now that the function has a different name the current package name makes more sense?

This comment has been minimized.

Copy link
@gziolo

gziolo Jun 3, 2019

Member

Nice, this updated function name reads better somehow 😃 Regarding the name of the package, it’s your call. We don’t have other media utils at the moment and it’s hard to tell what future will bring. Ideally, we could call it media but I guess it isn’t possible because of backward compatibility considerations.

This comment has been minimized.

Copy link
@noisysocks

noisysocks Jun 3, 2019

Member

Nice, this updated function name reads better somehow

It's because function names read nicer when they're a verb or begin with a verb 🙂

Show resolved Hide resolved packages/media-utils/package.json
Media upload util is a function that allows the invokers to upload files to the WordPress media library.
As an example, provided that `myFiles` is an array of file objects, `onFileChange` on onFileChange is a function that receives an array of objects containing the description of WordPress media items and `handleFileError` is a function that receives an object describing a possible error, the following code uploads a file to the WordPress media library:
```js
wp.mediaUtils.utils.mediaUpload( {

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

Those examples are tightly coupled to WordPress. It should be reflected in the description of the package that it won't work outside of WordPress instance.

See here: https://github.com/WordPress/gutenberg/blob/495608246d3ccdc8e3ff3271eee94d51474acb7e/packages/edit-post/README.md#edit-post

This package is meant to be used only with WordPress core. Feel free to use it in your own project but please keep in mind that it might never get fully documented.

* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import { components } from '@wordpress/media-utils';

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

I'm proposing something similar in my comment in a different place in this PR :)

@@ -13,6 +17,17 @@ import Layout from '../layout';
function EditWidgetsInitializer( { setupWidgetAreas, settings } ) {
useEffect( () => {
setupWidgetAreas();
addFilter(

This comment has been minimized.

Copy link
@gziolo

gziolo May 30, 2019

Member

We should pick one approach. It probably doesn't matter that much which one though. In terms of unit tests, this proposal might be even better.

@gziolo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

It's strange to me that we're using a filter for editor.MediaUpload when it is essentially a slot. Could this be a good opportunity to replace this filter with a Slot/Fill? cc. @gziolo

// packages/block-editor/src/components/media-upload/index.js

/**
 * WordPress dependencies
 */
import { createSlotFill, withFilters } from '@wordpress/components';
import { addAction } from '@wordpress/hooks';
import { deprecated } from '@wordpress/deprecated';

const { Slot, Fill } = createSlotFill( 'MediaUpload' );

function MediaUpload( props ) {
	return <Slot fillProps={ props } />;
}

MediaUpload.Fill = Fill;

addAction( 'hookAdded', 'core/block-editor/check-deprecated-media-upload', ( name ) => {
	if ( name === 'editor.MediaUpload' ) {
		deprecated( "addFilter( 'editor.MediaUpload', ... )", {
			alternative: '<MediaUpload.Fill>',
			plugin: 'Gutenberg',
		} );
	}
} );

export default withFilters( 'editor.MediaUpload', MediaUpload );
// packages/edit-post/src/components/layout/index.js

...
<MediaUpload.Fill>
	{ ( props ) => <MediaUploadImplementation { ...props } /> }
</MediaUpload.Fill>
...

It's a very interesting idea. I'd be nice to further explore. There might be a few things that would have to be addressed when using fills which aren't specific to this component only:

  • can you override or remove the currently rendered default fill?
  • what if there are multiple fills registered?

The nice part about this proposal that you operate fully in JSX syntax and you can override props with ease.

@noisysocks

This comment has been minimized.

Copy link
Member

commented May 31, 2019

It's a very interesting idea. I'd be nice to further explore.

👍 Not a blocker for this PR though.

@jorgefilipecosta jorgefilipecosta force-pushed the add/media-upload-widgets-screen branch from 69b8c47 to a22617a May 31, 2019

@jorgefilipecosta jorgefilipecosta force-pushed the add/media-upload-widgets-screen branch from 5661f74 to 2daa25a May 31, 2019

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

Thank you for the reviews and code updates/suggestions @noisysocks, @gziolo. The code was upload to match the suggestions provided 👍

@jorgefilipecosta jorgefilipecosta merged commit b710cc5 into master Jun 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/media-upload-widgets-screen branch Jun 3, 2019

@jorgefilipecosta jorgefilipecosta moved this from Generic Block Editor Issues to Legacy widgets issues/tasks in Widgets screen Jun 3, 2019

@jorgefilipecosta jorgefilipecosta moved this from Legacy widgets issues/tasks to Done in Widgets screen Jun 3, 2019

@youknowriad youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

Extract generic WordPress mediaUtils package. (WordPress#15521)
This PR extracts the mediaUpload function part of @wordpress/editor to an independent package and now calls it uploadMedia.
The MediaUpload component used to control WordPress media library is also part of this new package.
The package is used to implement media upload in the widgets screen.

jg314 added a commit to jg314/gutenberg that referenced this pull request Jul 19, 2019

Extract generic WordPress mediaUtils package. (WordPress#15521)
This PR extracts the mediaUpload function part of @wordpress/editor to an independent package and now calls it uploadMedia.
The MediaUpload component used to control WordPress media library is also part of this new package.
The package is used to implement media upload in the widgets screen.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.