Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#41 | 3096566-41.patch | 56.65 KB | xaqrox |
#33 | 3096566-33.patch | 56.57 KB | ravi.shankar |
#30 | Claro--Media-Library--Modal-image-media-add.png | 526.18 KB | huzooka |
#27 | interdiff-3096566-23-27.txt | 4.58 KB | huzooka |
#27 | claro-copy_media_library_styles-3096566-27.patch | 56.65 KB | huzooka |
Comments
Comment #2
Dinesh18 CreditAttribution: Dinesh18 as a volunteer and at Singapore Press Holdings commentedHere is the patch.
Comment #3
lauriiiThank you! We still need CSS and code from .theme file that is needed for media library to work. ✌️
Comment #4
aleevasWas added hooks from .theme file and css file for media library
Comment #5
huzookaSource file (
core/themes/claro/css/theme/media-library.pcss.css
) is missing.Comment #6
Dinesh18 CreditAttribution: Dinesh18 as a volunteer and at Singapore Press Holdings 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?
Comment #7
huzookaRe #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 runyarn build:css
in the./core
directory.So remaining tasks are:
media-library.css
tomedia-library.pcss.css
.media-library.pcss.css
. (You can runyarn lint:css
in the./core
directory.)yarn build:css
, and add both the.pcss.css
source and the compiled.css
files to the patch.Thanks!
Comment #8
aleevasAdded fix according to #7
Comment #9
phenaproximaThis will be a nice win for Claro (and the media library)! This issue could use a few screenshots, I think.
Comment #10
huzookaComment #11
huzookaComment #12
phenaproximaI think that looks great.
Comment #13
lauriiiShould 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.
Comment #14
huzookaComment #15
huzookaAddressing #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 thecompiledprocessed file directly in #11.Do we need a follow-up for removing the double selectors from
views-exposed-form.pcss.css
?(edited)
Comment #16
huzookaComment #17
huzookaComment #18
huzookaUpdated #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.Comment #19
huzookaComment #20
phenaproximaI 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.
This could use a comment, maybe? It's not really clear what this is doing.
"An empty library" isn't the most useful comment :) Can we expand it a bit to explain why we'd have this?
&$form should be type hinted as an array.
The first line of this doc comment isn't entirely accurate -- this targets the media_library view specifically.
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.)
Nit: The line breaks are a bit strange here. Can it all just be one cohesive paragraph?
Comment #21
lauriiiComment #22
huzookaComment #23
huzookaRe #20:
Comment #24
phenaproximaGave 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:
I don't think any of these need to block commit, but thought I'd call them out anyway.
Comment #25
lauriiiMoving to NW for #24.2.
Comment #26
huzookaComment #27
huzookaRe #24:
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.
Comment #28
huzookaComment #29
huzookaComment #30
huzookaComment #31
phenaproximaGave 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.
Comment #32
lauriiiComment #33
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have re-rolled the patch #27
Comment #34
nkoporecTested the latest re-roll and it works as expected. Marking as RTBC.
Comment #35
rpayanmComment #36
philosurfer CreditAttribution: philosurfer commentedBump... we need this ASAP :)
Comment #40
lauriiiTested 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.
Comment #41
xaqroxThe last patch doesn't apply to 8.8.1, so here's a patch to hold us over to the next release.