Problem/Motivation
The core/misc is very messy.
There are also loads of image files in there that a no longer used anywhere in core, most of them being replaced by #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core.
Proposed resolution
Remove all image files that are no longer used and move the forum icons into the forum module.
User interface changes
None
API changes
None
Original report by @jenlampton
- Move forum-icons.png into forum module
- Consolidate use of watchdog-warning.png and message-16-warning.png and remove one
- Remove all the "powered by drupal" logos if we truly don't care about SEO for drupal.org anymore
Remaining tasks
From comment #37: Split this entire issue into separate issues focusing on discrete image file-sets
Comment | File | Size | Author |
---|---|---|---|
#34 | 1356602-clean-up-misc-34.patch | 22.82 KB | mcjim |
#28 | 1356602-clean-up-misc-menu-expanded-28.patch | 317 bytes | rodrigoaguilera |
#27 | Screenshot 2014-03-25 12.18.12.jpg | 290.35 KB | LewisNyman |
#26 | 1356602-clean-up-misc-follow-up-26.patch | 581 bytes | mcjim |
#24 | Screen Shot 2014-03-25 at 08.33.09.png | 34.83 KB | KevinVanRansbeeck |
Comments
Comment #0.0
jenlamptonadded current location
Comment #1
jenlamptonRather than creating a bunch of separate issues, I'll lump together all the cleanup of misc in one issue.
Comment #2
sunThe second is covered by #1246722: Replace or rename redundant watchdog icons already.
For the third, there's #905988: Provide "Powered by Drupal"-Icons with transparent background
Closely related: #436500: Modernize generic menu icons/arrows in /misc
I think it would make more sense to focus this issue back on the forum icons only...?
Comment #3
mgiffordCan any of these get replaced by #1744278: Make use of jQuery UI Icons in core?
Do we have any SVG files for the existing images?
Comment #4
chrisjlee CreditAttribution: chrisjlee commentedPatch moves forum icon and updates forum css
Comment #5
chrisjlee CreditAttribution: chrisjlee commentedother issues:
- powered by drupal: Already have been removed from misc folder
- Consolidate warning and message-16.png: Not sure which to go yet. Probably the smaller message-16.png file
Comment #5.0
chrisjlee CreditAttribution: chrisjlee commentedadd more stuff that needs cleanup in misc
Comment #6
LewisNymanI think we could remove some of these icons now that we have #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core
Comment #7
LewisNymanList of icons that are no longer referenced in core:
... that's I bigger list than I was expecting! I doubled checked a few of these.
Comment #8
mcjim CreditAttribution: mcjim commentedI've combined the patch in #4 with deletion of the 22 files in #7.
The patch should work: the txt file should be easier to read (made using gif diff -D).
Comment #9
mgiffordThe bot likes it. What testing needs to happen to mark this RTBC?
Comment #10
sunComment #11
LewisNymanIs someone else able to sanity check this for me?
Comment #12
rteijeiro CreditAttribution: rteijeiro commentedOk, I only have found a couple of missing icons in node/add/article. I have opened a follow-up for that in #2208263: Missing icons in admin pages.
The rest is working for me. Maybe RTBC?
Comment #13
rteijeiro CreditAttribution: rteijeiro commentedGoing to re-add missing icons after discussing it with @lewisnyman.
Comment #14
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled patch and re-added menu-collapsed.png, menu-collapsed-rtl.png and help.png icons.
Comment #15
emma.mariaI applied #14 and clicked through many many pages to check if there are any 404 errors for images. I also enabled 5 modules that are not enabled by default to double check them incase anything was missed. I could not find any missing image errors so the patch is good.
Comment #16
webchickCould we please get an updated issue summary that explains what this patch is doing (and why)? I can't quite parse it out.
Comment #17
LewisNymanOk on it. How's that?
Comment #18
catchThis doesn't appear to modify the test that references grippie.png?
Comment #19
nicholasruunu CreditAttribution: nicholasruunu commentedHere you go @catch
- Drupal hero
Comment #20
LewisNymanComment #21
LewisNymanNice one! The patch still looks good.
Comment #22
webchickOk great, thanks!
Committed and pushed to 8.x.
Comment #24
KevinVanRansbeeck CreditAttribution: KevinVanRansbeeck commentedconfigure-dark.png seems to be used on the "Manage Display" page of a content type.
I am on latest git version (where the icons have been removed) and go this 404 image:
Comment #25
LewisNyman:(
Comment #26
mcjim CreditAttribution: mcjim commentedThis follow-up patch restores core/misc/configure-dark.png from commit fba6f74.
Comment #27
LewisNymanAh it wasn't in the CSS, that's maybe why it was missed. Here's a screenshot of it looking better.
Comment #28
rodrigoaguileraI think menu-expanded.png deleted in fab42693f81b93b4228162021ce916ef5feb13c7 should be restored since is giving me 404s and is referenced in:
Comment #29
LewisNymanI've confirmed the second followup patch. Both patches are RTBC.
Comment #30
sunLet's revert this patch entirely, please.
It clearly looks like image filenames were grepped in .css files only.
Comment #31
LewisNyman@sun That can't be it, because menu-expanded appears in css.
Are there any other files that we accidentally removed? If there aren't then it's a waste reverting it now... I'll take a look.
Comment #32
webchickHm. For now, I've rolled this back, because it looks like there's at least two spots that were missed (#26 and #28). We'll need one single patch with all of the proper files moved here and there as well.
Also tagging "needs manual testing" because yeah. ;)
Comment #34
mcjim CreditAttribution: mcjim commentedGrepping the codebase for references to the files in #7
configure-dark.pnggrippie.pnghelp.pngmenu-collapsed-rtl.pngmenu-collapsed.pngmenu-expanded.pngmessage-16-error.pngAmended list of files not referenced
Steps to create a new patch
Amending the 1356602-clean-up-misc-15.patch from #19, which adds/deleted/amends the following files:
core/misc/grippie.png can be removed as the test in
core/tests/Drupal/Tests/Core/Image/ImageTest.php
is amended in this patch to point to misc/help.png instead.The following files should not be removed for now as they are referenced in CSS or JS:
So, attached patch keeps those files. The following files are deleted:
Comment #35
sunFor the files (sets) using an optional suffix, the grep needs to account for dynamic string concatenations as in the following (made up) example:
E.g., like this:
So that result is apparently correct; only
message-16-error.png
is used.The same applies to files (sets) like
configure[-dark].png
, but in those cases, only the suffix might be dynamically removed via JavaScript, which means that the nameconfigure.png
may not necessarily appear in any grep results.Specifically regarding
configure[-dark].png
, here the idea is that core uses -dark by default, but if a theme uses a dark/negated color scheme, then it can easily replace the referenced image in CSS to use the non-dark version (which works for negated color schemes). →configure.png
is to be kept.You can get a good summary of all images referenced in /misc:
Doing that apparently yields:
...but yet, that file is listed as unused and to be deleted in #34, so something went wrong again.
Likewise:
Now, after looking at the patch, the last two mentioned points are addressed differently. Let's perform at least the forum-icons.png change in a separate issue (the patch for that should move/rename the existing file, instead of creating a new one).
Comment #36
zaporylieI think #1356602-34: [META] Clean up icons in misc is almost perfect, but I agree that forum-icons.png should be move from core/misc to core/modules/forum/images, instead of deleting/creating new one.
Comment #37
sunI wonder whether we shouldn't split this entire issue into separate issues focusing on discrete image file-sets?
That allows for proper testing + screenshots of each individual aspect; e.g., to confirm that message icons still appear correctly, or that forum icons still work correctly, etc. — It's close to impossible to ensure that with the currently taken approach.
Comment #38
emma.mariaComment #39
emma.mariaComment #40
LewisNymanIt would be good to at least clean this up a little before RC, I will try and create a few sub-issues soon.
Comment #41
LewisNymanThis issue just keeps on giving. I've created three sub issues. Let's get them in and see how misc looks after then:
#2358675: Remove messages icons in misc
#2358683: Move forum icons into the forum module
#2358685: Remove edit and configure icons from misc
Comment #42
LewisNymanWe should create a change record that covers all the child issues. See https://www.drupal.org/node/2358675#comment-9377411
Comment #43
LewisNymanI created some more child issues:
#2405057: Replace arrow-asc and arrow-desc images with Libricons and implement using CSS
#2405059: Replace menu images with Libricons
#2405061: Remove the unused permissions.png
Comment #58
quietone CreditAttribution: quietone at PreviousNext commentedThis issue was originally committed in 8.x but then reverted and work was done in child issues. All the child issue but one are complete. Therefor, closing this meta as fixed.
Thanks all!
Comment #60
quietone CreditAttribution: quietone at PreviousNext commentedThis does have a CR, removing tag.