Problem/Motivation

Media Library has been marked as stable in 8.8.x and as a result, most of the related styles were moved to Seven. Right now, if someone tries to use Media Library with Claro, it's unstyled for the most part which leads to poor UX.

Proposed resolution

Copy Seven Media Library styles to Claro as a starting point. Work on redesigning Media Library in follow-up issues.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#41 3096566-41.patch56.65 KBxaqrox
#33 3096566-33.patch56.57 KBravi.shankar
#30 Claro--Media-Library--Modal-image-media-add.png526.18 KBhuzooka
#27 interdiff-3096566-23-27.txt4.58 KBhuzooka
#27 claro-copy_media_library_styles-3096566-27.patch56.65 KBhuzooka
#27 Screenshot 2019-12-18 at 17.05.41.png108.21 KBhuzooka
#24 3096566-24.png155.21 KBphenaproxima
#23 interdiff-3096566-19-23.patch3.9 KBhuzooka
#23 claro-copy_media_library_styles-3096566-23.patch59.33 KBhuzooka
#19 interdiff-3096566-16-19.txt1.24 KBhuzooka
#19 claro-copy_media_library_styles-3096566-19.patch58.93 KBhuzooka
#16 interdiff-3096566-11-16.patch3.5 KBhuzooka
#16 claro-copy_media_library_styles-3096566-16.patch58.82 KBhuzooka
#11 Claro--Media-Library--Widget-with-weight.png377.08 KBhuzooka
#11 Claro--Media-Library--Widget-items.png365.56 KBhuzooka
#11 Claro--Media-Library--Widget-item-remove-ajax.png316.9 KBhuzooka
#11 Claro--Media-Library--Node-add.png143.18 KBhuzooka
#11 Claro--Media-Library--Modal-with-limit.png337.88 KBhuzooka
#11 Claro--Media-Library--Modal-multi-selection.png375.3 KBhuzooka
#11 Claro--Media-Library--Media-Grid.png391.19 KBhuzooka
#11 interdiff-3096566-8-11.txt19.7 KBhuzooka
#11 claro-copy_media_library_styles-3096566-11.patch59.08 KBhuzooka
#8 interdiff-4-8.txt19.82 KBaleevas
#8 3096566-8.patch50.64 KBaleevas
#4 interdiff_2-4.txt26.3 KBaleevas
#4 3096566-4.patch32.61 KBaleevas
#2 3096566-2.patch9.49 KBdinesh18

Comments

lauriii created an issue. See original summary.

dinesh18’s picture

dinesh18 commented
Status: Active » Needs review
StatusFileSize
new 3096566-2.patch9.49 KB

Here is the patch.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work

Thank you! We still need CSS and code from .theme file that is needed for media library to work. ✌️

aleevas’s picture

aleevas
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 3096566-4.patch32.61 KB
new interdiff_2-4.txt26.3 KB

Was added hooks from .theme file and css file for media library

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Needs work

Source file (core/themes/claro/css/theme/media-library.pcss.css) is missing.

dinesh18’s picture

dinesh18 commented

@huzooka what is actually .pcss.css means? I checked in seven themes but I didn't find any such file.
How do we add it?

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Re #6:

@Dinesh18, Claro uses PostCSS for compiling its CSS resources.
The file I mentioned in #5 is the source asset that will be compiled to core/themes/claro/css/theme/media-library.css if you run yarn build:css in the ./core directory.

So remaining tasks are:

  1. Rename the previously copied media-library.css to media-library.pcss.css.
  2. Check (and fix) the coding standards of media-library.pcss.css. (You can run yarn lint:css in the ./core directory.)
  3. Run yarn build:css, and add both the .pcss.css source and the compiled .css files to the patch.

Thanks!

aleevas’s picture

aleevas
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new 3096566-8.patch50.64 KB
new interdiff-4-8.txt19.82 KB

Added fix according to #7

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue tags: +Needs screenshots

This will be a nice win for Claro (and the media library)! This issue could use a few screenshots, I think.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
StatusFileSize
new claro-copy_media_library_styles-3096566-11.patch59.08 KB
new interdiff-3096566-8-11.txt19.7 KB
new Claro--Media-Library--Media-Grid.png391.19 KB
new Claro--Media-Library--Modal-multi-selection.png375.3 KB
new Claro--Media-Library--Modal-with-limit.png337.88 KB
new Claro--Media-Library--Node-add.png143.18 KB
new Claro--Media-Library--Widget-item-remove-ajax.png316.9 KB
new Claro--Media-Library--Widget-items.png365.56 KB
new Claro--Media-Library--Widget-with-weight.png377.08 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

I think that looks great.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented 
+++ b/core/themes/claro/css/components/views-exposed-form.css
@@ -54,10 +55,18 @@
+ * For (at least) Media Library, this file is typically inserted by AJAX add_css
+ * command when the dialog is opened.
+ * Since the add_css command does not care about CSS asset dependecies
+ * (nor load order). It adds the required (but missing) libraries to the
+ * beginnig of the HTML <head> uncondicionally (and thus make their selectors
+ * weaker than any else selectors in the preexisting assets), we have to double
+ * these selectors to get the expected output even for the Media Library modal.
  */

+++ b/core/themes/claro/css/components/views-exposed-form.pcss.css
@@ -1,14 +1,23 @@
+ * For (at least) Media Library, this file is typically inserted by AJAX add_css
+ * command when the dialog is opened.
+ * Since the add_css command does not care about CSS asset dependecies
+ * (nor load order). It adds the required (but missing) libraries to the
+ * beginnig of the HTML <head> uncondicionally (and thus make their selectors
+ * weaker than any else selectors in the preexisting assets), we have to double
+ * these selectors to get the expected output even for the Media Library modal.

Should we open a bug report for this? It sounds like a bug if the dependencies are not taken into account when CSS is inserted with AJAX.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
Status: Reviewed & tested by the community » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
Related issues: +#1461322: Fix AJAX add_css – insert the needed CSS assets after the already-inserted ones

Addressing #13 with a rephrased description and added the reference to the bug report.

I also fixed core/themes/claro/css/theme/media-library.pcss.css – I accidentally modified the compiled processed file directly in #11.

Do we need a follow-up for removing the double selectors from views-exposed-form.pcss.css?

(edited)

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new claro-copy_media_library_styles-3096566-16.patch58.82 KB
new interdiff-3096566-11-16.patch3.5 KB
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Updated #15 with the info about the follow-up in question.

@lauriii thinks that we don't need that follow-up, we only have to add a @todo comment.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
StatusFileSize
new claro-copy_media_library_styles-3096566-19.patch58.93 KB
new interdiff-3096566-16-19.txt1.24 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

I haven't tested this in a browser -- I want to before I RTBC -- but the code looks okay to me, except for some minor documentation nits.

  1. +++ b/core/themes/claro/claro.info.yml
@@ -120,6 +120,8 @@ libraries-override:
+  classy/media_library: claro/empty
+

    This could use a comment, maybe? It's not really clear what this is doing. 

  2. +++ b/core/themes/claro/claro.libraries.yml
@@ -64,6 +64,12 @@ global-styling:
+# An empty library.
+# @see https://www.drupal.org/node/3098375
+empty:
+  version: VERSION
+  css: {}

    "An empty library" isn't the most useful comment :) Can we expand it a bit to explain why we'd have this? 

  3. +++ b/core/themes/claro/claro.theme
@@ -352,13 +353,22 @@ function claro_preprocess_details(&$variables) {
+function claro_form_alter(&$form, FormStateInterface $form_state, $form_id) {

    &$form should be type hinted as an array. 

  4. +++ b/core/themes/claro/claro.theme
@@ -1202,3 +1219,171 @@ function claro_preprocess_image_widget(&$variables) {
+ * Implements hook_preprocess_views_view_fields().
+ *
+ * This targets each rendered media item in the grid display of the media
+ * library's modal dialog.
+ */
+function claro_preprocess_views_view_fields__media_library(array &$variables) {

    The first line of this doc comment isn't entirely accurate -- this targets the media_library view specifically. 

  5. +++ b/core/themes/claro/claro.theme
@@ -1202,3 +1219,171 @@ function claro_preprocess_image_widget(&$variables) {
+  $add_classes = function (&$option, array $classes_to_add) {
+    $classes = preg_split('/\s+/', $option);
+    $classes = array_filter($classes);
+    $classes = array_merge($classes, $classes_to_add);
+    $option = implode(' ', array_unique($classes));
+  };

    Maybe worth a follow-up -- this should be a method of all Views handlers that accept CSS classes. (Maybe via a new trait in Views.) 

  6. +++ b/core/themes/claro/css/components/views-exposed-form.css
@@ -54,10 +55,22 @@
+ * For (at least) Media Library, this file is typically inserted by AJAX add_css
+ * command when the dialog is opened.
+ * The AJAX add_css command always adds the missing-but-required CSS assets to
+ * the beginning of the HTML <head>.
+ * Because of this, we cannot rely on the expected loading order of the
+ * CSS assets.
+ * This is why we have to double these selectors: we have to get the expected
+ * output even for the Media Library modal.

    Nit: The line breaks are a bit strange here. Can it all just be one cohesive paragraph?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
StatusFileSize
new claro-copy_media_library_styles-3096566-23.patch59.33 KB
new interdiff-3096566-19-23.patch3.9 KB

Re #20:

  1. Done.
  2. Done.
  3. Done.
  4. This was copied from Seven.
  5. ...just like this one.
  6. Done.
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 3096566-24.png155.21 KB

Gave this a quick test. I added a media field to the Article content type, and used it to add some media to the media library and select it in Claro. Without the patch, it looked bad and confusing -- borderline unusable, even. After the patch, it was vastly improved. Nice work fixing this!

I did notice three things:

  1. In the media library, the "Grid/Table" switcher is below the exposed filters. In Seven, they are to the right of the exposed filters. Is this intentional?
  2. When adding media, there is a shadow around the "Remove" button which maybe shouldn't be there. See attached screenshot.
  3. In Claro, the media library modal feels a bit cramped compared to Seven. I'm guessing that's because Claro's design gives a lot of elements more breathing room than they normally have in Seven (which is a good thing). That said, I'm not sure this is something Claro can fix even if it wanted to; Media Library should probably allow calling code to choose how big the modal should be. Maybe that calls for a follow-up?

I don't think any of these need to block commit, but thought I'd call them out anyway.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work

Moving to NW for #24.2.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: Unassigned » huzooka
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Related issues: +#3096241: Refactor image and file field widgets to improve contrib compatibility and to make their templates and preprocess functions DRY
StatusFileSize
new Screenshot 2019-12-18 at 17.05.41.png108.21 KB
new claro-copy_media_library_styles-3096566-27.patch56.65 KB
new interdiff-3096566-23-27.txt4.58 KB

Re #24:

  1. Claro displays views exposed filters as a standalone component, with borders and padding around it's items. Since we don't have any designs for Media Library (we're just copying styles), this is the less disruptive way to display these comps.
  2. I completely removed the media-library-provided overrides from that button, and only kept the positioning ones.
  3. I think that the size of the modal shoudn't be controlled so strictly by a module.
    If the user tries to use the embed dialog from a mobile device (with a browser that's canvas width is less than 600 pixels), it is close to be unusable. And this is happening even using Seven, Bartik, Classy, Stable or Stark themes.

Added #3096241: Refactor image and file field widgets to improve contrib compatibility and to make their templates and preprocess functions DRY as related issue.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Needs work » Needs review
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Related issues: +#3102007: Copy the media library styles from the Seven theme, +#3102011: Media Library's embed modal is almost unusable with less than 600px browser viewport width
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new Claro--Media-Library--Modal-image-media-add.png526.18 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

Gave this another quick manual test on 8.8.x HEAD and I think it looked great. It's a bit sad to lose the nice styling of the "remove" button for an uploaded media item, but that's actually fine with me; it should fit Claro's design aesthetic, not Seven's.

So, I think this looks terrific and I'd like to see it land! RTBC.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3096566-33.patch56.57 KB

I have re-rolled the patch #27

nkoporec’s picture

nkoporec
Primary language English
Location Slovenia
commented
Status: Needs review » Reviewed & tested by the community

Tested the latest re-roll and it works as expected. Marking as RTBC.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Issue tags: -Needs reroll
philosurfer’s picture

philosurfer commented

Bump... we need this ASAP :)

  • lauriii committed 389ca89 on 9.0.x 
    Issue #3096566 by huzooka, aleevas, Dinesh18, ravi.shankar, phenaproxima...

  • lauriii committed acc50d8 on 8.9.x 
    Issue #3096566 by huzooka, aleevas, Dinesh18, ravi.shankar, phenaproxima...

  • lauriii committed 994deab on 8.8.x 
    Issue #3096566 by huzooka, aleevas, Dinesh18, ravi.shankar, phenaproxima...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Tested this manually and this is a very nice improvement! I also reviewed this with git diff -C -C and paid close attention to the changes that were not copied. I also manually confirmed that everything was copied, and confirmed that the things left out were left out for a valid reason.

Committed 389ca89 and pushed to 9.0.x, 8.9.x and 8.8.x. Thanks!

Next step: #3062751: Media and media library.

xaqrox’s picture

xaqrox
he/him
Primary language English
Location Washington, D.C.
commented
StatusFileSize
new 3096566-41.patch56.65 KB

The last patch doesn't apply to 8.8.1, so here's a patch to hold us over to the next release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.