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

Image Block: Integrate with the media upload modal #802

Merged
merged 6 commits into from
May 17, 2017

Conversation

youknowriad
Copy link
Contributor

closes #418

This PR integrates the image block with the media uploader. I'm not familiar with wp.media, so I'm just following https://codex.wordpress.org/Javascript_Reference/wp.media here and it seems to work as expected.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label May 16, 2017
@youknowriad youknowriad self-assigned this May 16, 2017
} );

// When an image is selected in the media frame...
this.frame.on( 'select', this.onSelect );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a separate component that block authors can more easily add? It'd be nice if we could keep the complexity of the image block to a minimum, and abstract away the need for hooking up the media modal.

@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label May 16, 2017
@jasmussen
Copy link
Contributor

User experience wise, this is working great! Thank you for doing this.

There was also some discussion a while back for auto-invoking the media library when you insert an image from the inserter directly. So this would be the behavior:

  1. click inserter, click image
  2. placeholder is inserted and the media library is quickly opened
    3a. you pick an image, insert, done
    3b. you close the media modal and are left with the placeholder as it is now

@youknowriad
Copy link
Contributor Author

@jasmussen The workflow should be good now 👍

@jasmussen
Copy link
Contributor

@jasmussen The workflow should be good now 👍

Yes! This validates your idea. Works really really well!

import { __ } from 'i18n';
import Button from 'components/button';

class MediaUploadButton extends Component {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put this at the same abstraction level as Editable, not in general UI components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I hesitated but I decided to put it here because it can be useful outside the Editor (in other parts of WordPress).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocks will be potentially usable outside the editor as well, so I'd still keep it with Editable :)

@westonruter
Copy link
Member

Please also refer to the new Image widget for some patterns for integration with the media library:

@youknowriad
Copy link
Contributor Author

@westonruter Any specific concerned you faced when integrating with the Media Library in the Image widget? The simple integration here seems to work well, at least for now.

@westonruter
Copy link
Member

@youknowriad there were quite a few things we ran up against when doing more extensive integrations with the media library. If all you need is a simple integration, then cool. But as you look for deeper integration and making use of more features, please refer to the Image widget for prior learnings and collaboration.

@mtias
Copy link
Member

mtias commented May 16, 2017

Let's merge this.

@jasmussen playing with it a bit I kind of like the ability to insert an empty placeholder first, and then choosing to upload or pick from library.

@jasmussen
Copy link
Contributor

Agreed on merging, then we can decide later on whether we want it this or that way. I like that it's similar to inserting images today. But would be good to feel.

if ( !! type ) {
frameConfig.library = { type };
}
this.frame = wp.media( frameConfig );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to clean up this frame when the component unmounts? Maybe via the view.remove() function?

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/js/media/views/view.js?rev=33337#L52

@youknowriad youknowriad merged commit a8b0eb6 into master May 17, 2017
@youknowriad youknowriad deleted the add/media-upload branch May 17, 2017 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Figure out media integration.
5 participants