WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 11 months ago

#36916 new enhancement

Refactor EditAttachments frame to work outside Media Library admin page

Reported by: Funkatronic Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.5.2
Component: Media Keywords: has-patch needs-testing
Focuses: javascript, administration Cc:

Description

The EditAttachments frame/modal is used on the Media Library page to display and edit meta data of attachments. This modal could be useful for plugin/theme developers but a few things tightly couple it to the Media Library:

  1. The frame requires a router. This is easily remedied by feeding it a dummy object with two functions it seems to need but I can see this easily refactored using event listeners
  1. When opened, the code in the modal deletes all mediaelement.js instances on a page. Not a problem in the media library but if you want to use it on a page with video and audio elements (which I do) it deletes them all from the DOM

Attachments (2)

media-grid.js.patch (2.7 KB) - added by Funkatronic 18 months ago.
Decoupled EditAttachment and Manage frames
edit-attachment-test.js (782 bytes) - added by Funkatronic 18 months ago.
Simplified version of usage

Download all attachments as: .zip

Change History (10)

@Funkatronic
18 months ago

Decoupled EditAttachment and Manage frames

#1 @Funkatronic
18 months ago

  • Keywords has-patch needs-testing added

Added patch. Refactored so

  1. gridRouter logic moved to Manage frame instead of inside EditAttachments
  2. TwoColumns view no longer deletes all MediaElement.js instances, just the ones in the view
Last edited 18 months ago by Funkatronic (previous) (diff)

#2 @Funkatronic
18 months ago

  • Type changed from feature request to enhancement

#3 @adamsilverstein
18 months ago

Hey @Funkatronic, thanks for the suggestion and the patch!

Looking at the patch briefly it seems reasonable, can you provide a code snippet showing how you would use the edit dialog demonstrating how the patch helps you? My main concern with this change is that we might unexpectedly break the routing behavior (for example re/loading a url in the edit mode). I don't think we have good unit tests to verify this, so we need to test carefully.

#4 @Funkatronic
18 months ago

Thank for the reply @adamsilverstein. So I help out with the Meta Box Plugin and we have a few media based fields; one that a user selects files in general, one that user selects images and now I'm working on ones that do videos and audio. Each attachment chosen has a link to the admin page of that attachment but I just added the ability to use the EditAttachments modal. It keeps the user on the same page and allows them to edit the metadata of the attachments if and when needed, as well as giving them a larger preview of the attachment, in the case of images and videos. Only problem is that to make it work, I have to feed it a dummy router that has noop functions so that it even opens outside the media library page. I don't need a router for a meta box but the modal is very handy. The code I refactored hooks to the same events the original function calls were inside, so it should and does work. Haven't had any issues with the urls the router creates; it opens up the right attachments when navigated.

The other issue with the videos being deleted was a dealbreaker and it forced me to extend two views to make it not do that.

@Funkatronic
18 months ago

Simplified version of usage

#5 @Funkatronic
18 months ago

So I've tested this on my local machine but I want to do a unit test. Is there documentation on how to create one?

#6 @adamsilverstein
18 months ago

@Funkatronic So far I think we mainly have 'functional' tests which may be enough to test things here; JS unit tests are pretty new to core and we have very poor coverage :( Here is the post I found where we introduced JS unit tests, it describes how to create them briefly:

https://make.wordpress.org/core/2013/09/13/javascript-unit-tests-for-core/

Thanks for the code snippet, I will give that a test.

#7 @Funkatronic
18 months ago

@adamsilverstein The code is what I have to do right now before patch. After the patch, you can just remove the dummy gridRouter

#8 @Funkatronic
11 months ago

Sorry, It's been a while since I touched this. Would it be possible to branch out a new version of this modal that isn't so coupled to the media library page? One that a developer can use just for giving details of media either on their own or part of a collection?

Note: See TracTickets for help on using tickets.