Problem/Motivation

For Drupal 8.3, we want to ship with a library that allows displaying and selecting from a listing of previously-uploaded media.

Proposed resolution

Implement an MVP media library, based on the user stories and design from #2796001: [prototype] Create design for a Media Library.

Implementation happening so far at https://github.com/mortenson/media_library, will post patches later.

Issues with patches you'll need to apply to test this issue:

- #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well
- #2831274: Bring Media entity module to core as Media module

Remaining tasks

These are fairly high level, but in general I think we should:

  1. Discuss what should be covered by the MVP (this is being done in #2796001: [prototype] Create design for a Media Library).
  2. Develop a pattern for selecting entities in views (this doesn't have to be a form element/widget, just the logic that powers the MVP)
  3. Develop a pattern for adding (uploading) new media, with a focus on the "image" bundle
  4. Write CSS and JS required for the MVP to function
  5. Ensure that the library works in a standalone page at /admin/content/media
  6. Ensure that the library works in Field widgets for selecting Media
  7. Ensure that the library works in Field widgets for uploading/creating Media
  8. Ensure that the library works in WYSIWYG
  9. Write Javascript test coverage for the Field widget and WYSIWYG functionality

User interface changes

This is a UI addition, so existing user interfaces should not be affected by this change.

API changes

We may be implementing new APIs for implementing Views-based entity selection, but we haven't decided how abstract we're going to get (see task #2 above).

Data model changes

None, the data model changes are being added in #2831274: Bring Media entity module to core as Media module.

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

Issue summary: View changes
samuel.mortenson’s picture

Issue summary: View changes
samuel.mortenson’s picture

We're working on an experimental Github project before pulling a patch, which is located here: https://github.com/mortenson/media_library

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: +sprint
samuel.mortenson’s picture

Status: Active » Needs work
FileSize
70.29 KB
597.83 KB
368.05 KB

As the Berlin Media Sprint is closing up, I'm uploading a patch of what we have now and have marked the Github project as deprecated, as all development should happen in the issue queues from now on. This patch was worked on by myself and @webflo.

Here's what the module provides so far:

  1. A View called "Media" which visually lists all the media on your site.
  2. An admin page for this View which can be found at /admin/content/media, and the Local Task that links to that page.
  3. An Action Link to allow users to quickly add media from the standalone page.
  4. Derived Local Tasks that allow users to drill down into the library by Type. Each local task corresponds to a contextual filter that is passed to the standalone page (ex: /admin/content/media/video).
  5. A new View Mode for Media Types, specifically designed and styled to look presentable in the Media Library. This also includes a default template and styling (CSS).
  6. Javascript behaviors to allow users to click each Media Item and in turn select it for use in the Bulk Operations form.
  7. Javascript behaviors to persist View selection across multiple pages, filters, etc. This means that you can select a Media Item, browse to a new page, and the selection will still be carried over when submitting the bulk form.
  8. Javascript behaviors to carry on the persistent View selection and extra data from our widget to our custom Views Field.
  9. A new Views select form, similar to Bulk Operations, specifically written for use in our Field Widget.
  10. A new Field Widget which allows users to select Media Items using the Media Library. This could be abstracted to work for any View/Entity Type.

I've also uploaded two animated GIFs showing some of the functionality already "completed":

Media Library standalone page

Media Library field widget

The following core patches are required to use this patch:

  1. #2834801: Schema validation fails on the "entity" argument_validator when the "bundles" form key is not set
  2. #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well

To facilitate testing and review of this patch, I've added a test submodule called "media_library_test". This module provides two default Media Types, and a Content Type which uses a single and multivalue Media field. I recommend following this flow when testing:

  1. Apply the required core patches (see above).
  2. Enable the "media_library_test" module.
  3. Visit /media/add/type_one, fill in the "Name" field, and submit the form.
  4. Visit /media/add/type_two, fill in the "Name" field, and submit the form.
  5. Visit /admin/content/media and play around with the tabs, filters, and bulk select form.
  6. Visit /node/add/media_library_test and try selecting some media in the two fields. It's worth noting that sorting is not yet implemented in this patch.

There is so much work to be done here, but hopefully this list of major priorities makes sense:

  1. Review the Field Widget, as we've never used a View in a Field Widget before (in core) and I'm not an expert at writing these, and improve it where necessary.
  2. Add drag+drop sorting to the Field Widget to re-order Media Items.
  3. Implement a CKEditor button that allows for embedding Media directly in WYSIWYG.
  4. Add tabs that are similar to /admin/content/media to the Field Widget modal, which should use AJAX to reload the modal with the appropriate contextual filters.
  5. Think about how validation plays into using a View to select Media - if I only allow specific Media Types in my Field, should the View dynamically filter based on those Types?
  6. The mockups showed that you can also add (create, not re-use) Media from the modal - how are we going to accomplish this?
  7. What is expected when the View selection is submitted in the Field Widget - should the new selection replace the existing selection or append to it?
  8. What is expected in the MVP in terms of features - can the MVP just be about re-using existing media like the autocomplete widget?

This patch is by no means ready for a RTBC status, so I'm marking the issue as "Needs work" until we've added all the features we want for the MVP. That said, I would love to get feedback at this stage and would appreciate some help from core contributors who are more experienced than me.

Thank you!

mparker17’s picture

@samuel.mortenson Fantastic work! :) Thank you for helping to move this issue forward!

webflo’s picture

webflo’s picture

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -317,15 +317,15 @@ public static function addItems($form, FormStateInterface $form_state) {
-            ++$field_state['items_count'];

I realize that, this is not correct. items_count and delta should match. I think we should init items_count with 0 and increase it similar to $delta. or use $delta for the comparison.

Anyway, i added and remove media entities in the widget and the field state was not correct after a few iterations.

webflo’s picture

We should decide if we want to replace the existing selection or always add new items. Its an odd mix atm.

samuel.mortenson’s picture

@webflo Agreed, I think the View should only add items onto the existing selection, but it would be good to get UX feedback on what is expected.

I would propose these changes to the widget, if we go with appending and not replacing:

  1. Change the button that opens the modal to say "Add Media"
  2. When a selection is made, append the selection onto the existing items and call $form_state->setRebuild()
  3. Split updateWidget into two new methods - one for appending items, and one for removal. These would return AJAX commands to remove/add specific items, instead of re-rendering the entire selection area. This allows us to have the widget be sortable without making any extra AJAX requests when you re-sort items.

I'll work on this and try to get a new patch in soon with sorting working.

samuel.mortenson’s picture

FileSize
72.75 KB
10.4 KB

This patch:

  1. Removes my use of getUserInput and properly uses form values and/or field state
  2. Adds sorting to items using fairly minimal JS/form logic
  3. Ensures that Media Items you add from the View are always appended to the existing selection
  4. Changes the style of the "Remove" button to use an icon - which looks kind of lame right now but I assume we can get some UX design resources to make something nicer.
samuel.mortenson’s picture

Some quick self-review:

  1. +++ b/core/modules/media_library/icons/checkbox.svg
    @@ -0,0 +1,4 @@
    +<svg fill="#91b6f9" height="24" viewBox="0 0 24 24" width="24" xmlns="http://www.w3.org/2000/svg">
    

    We need to use the libricons mentioned in #2828538: Produce high fidelity screens based on Media prototype.

  2. +++ b/core/modules/media_library/js/media_library.view.js
    @@ -0,0 +1,118 @@
    +  var MediaLibrarySelectedEntities = {};
    

    The persistent selection is really useful for AJAX views - but isn't required for the Media Library to function. @webflo Should this be moved to a new issue?

  3. +++ b/core/modules/media_library/media_library.info.yml
    @@ -0,0 +1,10 @@
    +name: 'Media library'
    

    We need a composer.json change as well, since we're adding a new module to core.

  4. +++ b/core/modules/media_library/media_library.libraries.yml
    @@ -0,0 +1,28 @@
    +      css/media_library.css: {}
    

    We'll eventually need to break this out into functional/theme styles, and copy the theme styles into stable.

  5. +++ b/core/modules/media_library/media_library.module
    @@ -0,0 +1,73 @@
    +function _media_library_pretty_time($time) {
    

    Just a comment - This was added following UX feedback about having relative dates in the View, the only difference between this and the "datetime_time_ago" field formatter is that I set a max age on using relative times. If a Media Item was created over a month ago, we show the full date.

  6. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,365 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    

    This function is really long! Maybe we can split it up into protected methods to better support refactors and tests.

  7. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -0,0 +1,365 @@
    +  public static function removeItem($form, FormStateInterface $form_state) {
    

    I'm not 100% sure this callback works now that we have sorting in. When we add Javascript testing I'll feel better about the logic here.

In general I think the next step here should be coordination with the UX team to identify MVP goals, and we should start writing tests for the standalone page and the widget before adding entirely new features. I'll plan on writing Javascript tests for the widget this week.

Gábor Hojtsy’s picture

@samuel.mortenson: re the remove icon, we use libricons for admin facing styling, see https://github.com/ry5n/libricons -- if that has a useful icon for your needs, that would be the most consistent pick.

Gábor Hojtsy’s picture

Tried applying this patch with https://www.drupal.org/node/2831274 #162 and it says

Fatal error: Class 'Drupal\system\Plugin\views\field\SelectFormBase' not found in /home/d1dti/www/core/modules/media_library/src/Plugin/views/field/SelectMedia.php on line 23

upon installing the modules. Should one also apply #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well or that is not the latest of what to apply?

Gábor Hojtsy’s picture

+++ b/core/modules/media_library/tests/modules/media_library_test/media_library_test.info.yml
@@ -0,0 +1,9 @@
+package: Testing

If you can make this not be in the testing package, drive-by testers using simplytest.me could run with the patch and try it out with the test types. That would not be kept for the final version of the patch of course.

samuel.mortenson’s picture

#15 - You'll also need to apply the most recent patch from #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well, yes. I gave some review instructions in #6 but maybe that comment should be re-factored into the issue summary.

I noticed that in #2660124: Dynamically generate layout icons based on well formed config contributors are uploading full patches (including dependencies) for the testbot, then do-not-test patches with just the changes from that issue. I'll start using that format going forward to make review easier.

#16 - Good point, I'll try to get that into the next patch.

Berdir’s picture

#16: Doesn't work like that, it is not the package that is being excluded. Drupal doesn't look for modules in tests folder unless some flag is setting settings.php or we're actually running a test. So if you want to make it available, you have to move it to a separate folder, outside of tests.

Gábor Hojtsy’s picture

@Berdir: thanks. I think it would be useful to have it in a regular module location then to facilitate testing. Alternatively if the document/image/video patches are in a good enough shape, we maybe want to have a compound testing issue for all of them somewhere :) Asking that now on #drupal-media in IRC.

samuel.mortenson’s picture

I attended the UX meeting today and the feedback on way we're handling re-sorting Media was very positive. I think it's safe to say that the way sorting the selection works now is going to be similar to what eventually lands in core.

That said, I still need to write Javascript tests for the field widget as it exists now, make a few styling updates that were suggested by the UX team, and continue to improve the field widget's handling of selections. We'll also need to provide default display mode settings for known Media Types that are going to be committed in core, specifically YouTube, File, and Image. Things are looking good so far though!

Gábor Hojtsy’s picture

Youtube recording from UX meeting today: http://youtu.be/hf8AovBZflo

samuel.mortenson’s picture

Sorry for the holiday delay! I'm still working on tests, and have started looking into a CKEditor plugin as well. My notes from #13 still need addressed, and I also noticed some quirks when adding/removing/resorting in the field widget. We're also waiting on new icons for edit/remove from the UX team.

samuel.mortenson’s picture

FileSize
75.23 KB
2.7 KB

I started working on integration tests and already got stuck! For some reason clicking on the local tasks in the admin view returns a 403, but I can't replicate this in the UI at all. If anyone is a test expert and can figure out why this minimal test fails, let me know! Otherwise I'm waiting for new icons to follow up with the rest of my feedback from #13.

For other contributors, the open tasks for this issue are:

1) Find out why the test in this patch fails
2) Figure out what's required to embed Media in WYSIWYG as a part of this or another patch, and write it :-)
3) Review the Field Widget added in this patch, see if there are bugs when adding/sorting/removing media and address them

Berdir’s picture

Your views argument includes a view access check for the passed in media type. But media_type only has an admin permission (administer media types), which your test user doesn't have.

It is an interesting feature obviously, but per-type view access is not something that the current media patch/module provides.

Gábor Hojtsy’s picture

@samuel.morteson: the embedding problem has its issue at #2801307: Support WYSIWYG embedding of entities other than files, I see how it needs to be integrated with the library, but it also needs its own components, eg. markup decisions for the embed code, etc.

samuel.mortenson’s picture

@berdir That makes sense, thank you! I'm all set to continue writing tests for the library and field widget now. I'll probably remove the type access check as there is already (view) permission checking on the media items, and a user shouldn't need to administer media types to use the library.

@gábor-hojtsy It makes sense that the WYSIWYG part of this issue would be blocked until that's closed, I'll hold off on that work for now.

So I'll just work on test coverage for now, then circle back to the field widget. Thanks again!

tedbow’s picture

Issue summary: View changes

Crossed out github link in summary because it seems patches are more recent.

Also added link to #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well in the summary so that it is obvious you need to apply this patch too.

samuel.mortenson’s picture

Issue summary: View changes
samuel.mortenson’s picture

Just getting test coverage working for the standalone page, field widget tests are forthcoming. The "with-dependencies" patch includes the two issues this patch depends on, so please only review the "do-not-test" patch if you're looking into this issue. :)

The last submitted patch, 29: media-library-with-dependencies-2834729-29.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 29: media-library-with-dependencies-2834729-29.patch, failed testing.

slashrsm’s picture

#2830691: Improve out of the box UI / UX for Media handling from the contrib Media module is related and could be useful.

Gábor Hojtsy’s picture

@dyannenova is working on a significantly updated patch for this. (I don't have a way to assign this to her but it should be).

Also we discussed the latest version of that on the UX call today. Here is the recording (very short, just reviewed this): https://youtu.be/LbQis7Va8zg

DyanneNova’s picture

Here are my updates on top of the dependencies from #29. This includes styling changes for the media library, and interaction changes, like the toggle for metadata, and hiding actions on the main library page until items are selected.

Here's an example of attaching new media through the library field widget:
attaching media interaction

Work that needs to be done

I've added an example button for adding new media to the top of the library widget, but this needs to be built out to load the form in the modal and lead the user through adding media and back to the library with their newly added item selected.
add new media interaction

Right now the core dialog javascript moves the views filter submit down to the bottom of the dialog. For clarity, this needs to be overridden to move the "Apply filters" button back to the header. The text on the "Selected items" button could also be clearer. On the ux call we discussed using "Attach selected media" instead.
dialog buttons

Status: Needs review » Needs work

The last submitted patch, 35: media-library-with-dependencies-2834729-35.patch, failed testing.

webflo’s picture

@DyanneNova: Could you upload the last patch without all the dependencies? Thx.

samuel.mortenson’s picture

diff -u b/core/modules/media_library/media_library.links.action.yml b/core/modules/media_library/media_library.links.action.yml
--- b/core/modules/media_library/media_library.links.action.yml
+++ b/core/modules/media_library/media_library.links.action.yml
@@ -6 +5,0 @@
-    - view.media_library.bundle_page
reverted:
--- b/core/modules/media_library/media_library.links.task.yml
+++ /dev/null
@@ -1,8 +0,0 @@
-media_library.local_tasks:
-  deriver: 'Drupal\media_library\Plugin\Derivative\MediaLibraryLocalTasks'
-  weight: 1
-
-media_library.local_tasks.all:
-  title: 'All categories'
-  route_name: view.media_library.page
-  parent_id: views_view:view.media_library.page

Why was the local task deriver removed?

diff -u b/core/modules/media_library/templates/media--media-library.html.twig b/core/modules/media_library/templates/media--media-library.html.twig
--- b/core/modules/media_library/templates/media--media-library.html.twig
+++ b/core/modules/media_library/templates/media--media-library.html.twig
@@ -45,8 +45,6 @@
       <div class="media-library-item-name">{{ name }}</div>
       <div class="media-library-item-created">{{ created_ago }}</div>
-      <div class="media-library-item-author">{{ author_info }}</div>
+      <div class="media-library-item-type">{{ media.bundle()|title }}</div>
     </div>
-    {# This is used to prevent dragging nested elements in the widget. #}
-    <div class="media-library-mask"></div>
   {% endif %}
 </article>

And why was the mask removed?

DyanneNova’s picture

The local tasks were removed based on one of the ux meeting discussions, where everyone agreed that they should be changed to views filters.

I removed the mask because I couldn't find a case where it was needed. Where is it being used? Is there a reason we can't target only the top level for dragging?

samuel.mortenson’s picture

@DyanneNova Alright, we'll also need to update MediaLibraryTest as that specifically had coverage for the local tasks.

I found that while re-ordering media items in the field widget using drag and drop, users would sometimes drag the thumbnail image, preventing the default jQuery.ui draggable behavior. You should be able to replicate this by selecting two Media items with thumbnails and re-ordering them while dragging on a thumbnail.

samuel.mortenson’s picture

Issue summary: View changes
Berdir’s picture

Component: entity system » media system

I think this makes more sense in the media system component, probably the issue was created before that existed?

seanB’s picture

Rerolled since the main patch landed. I tried to create a interdiff for 29-35 and used that for the reroll, uploading that as well. Found a new issue for the main patch #2880199: Revision user not set when saving media which causes this to break. So hopefully we can that fixed asap.

*edit
In the end everything works by the way! The admin/content/media page is a little weird (since you can't seem to do anything like edit or delete media items). But picking stuff from the library looks awesome! Nice work!

idebr’s picture

Enabling Media library media-library-with-dependencies-2834729-43.patch results in a fatal error:

Fatal error: Class 'Drupal\system\Plugin\views\field\SelectFormBase' not found in /[...]/core/modules/media_library/src/Plugin/views/field/SelectMedia.php on line 23

seanB’s picture

You need to apply #2834777: Refactor Drupal\system\Plugin\views\field\BulkForm to support a select form as well first. I guess that patch should be added to the dependencies patch.