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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Dinesh18’s picture

Status: Active » Needs review
FileSize
9.49 KB

Here is the patch.

lauriii’s picture

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

Status: Needs work » Needs review
FileSize
32.61 KB
26.3 KB

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

huzooka’s picture

Status: Needs review » Needs work

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

Dinesh18’s picture

@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

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

Status: Needs work » Needs review
FileSize
50.64 KB
19.82 KB

Added fix according to #7

phenaproxima’s picture

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

Assigned: Unassigned » huzooka
Status: Needs review » Needs work
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

I think that looks great.

lauriii’s picture

+++ 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

Assigned: Unassigned » huzooka
Status: Reviewed & tested by the community » Needs work
huzooka’s picture

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’s picture

Assigned: Unassigned » huzooka
Status: Needs review » Needs work
huzooka’s picture

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

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
FileSize
58.93 KB
1.24 KB
phenaproxima’s picture

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

Status: Needs review » Needs work
huzooka’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

Re #20:

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

FileSize
155.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

Status: Needs review » Needs work

Moving to NW for #24.2.

huzooka’s picture

Assigned: Unassigned » huzooka
huzooka’s picture

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

Assigned: huzooka » Unassigned
Status: Needs work » Needs review
huzooka’s picture

huzooka’s picture

phenaproxima’s picture

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

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
56.57 KB

I have re-rolled the patch #27

nkoporec’s picture

Status: Needs review » Reviewed & tested by the community

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

rpayanm’s picture

Issue tags: -Needs reroll
philosurfer’s picture

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

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

FileSize
56.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.