In #2075949: Add Libricons to messages, we came to the conclusion that the dblog module should update icon usage to SVG, following the new practices for implementation (see #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core).

This module uses the file "message-16-warning.png" at line 220 in system.admin.css and "message-16-error.png" at line 217 in system.admin.css

Test Pages

  • /admin/reports/status
  • /admin/modules - open a module that has links
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klonos’s picture

Title: Update use of icons to new standards » system.module: Update use of icons to new standards

...less vague title (there are at least 3 issues with the same name: #2075949-10: Add Libricons to messages).

LewisNyman’s picture

tagging

Outi’s picture

Status: Active » Needs review
FileSize
7.82 KB

Replacing the icons on the system.admin.css by the libricon icons. (The draggable icon in system.module.css is not changed.)

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/css/system.admin.css
@@ -214,10 +223,16 @@ table.system-status-report td.status-icon div {
+  background-image: url(../../../misc/e29700/warning.png);

Looks like these paths are missing the ‘icons’ folder

LewisNyman’s picture

Issue summary: View changes

Added more info

Outi’s picture

Oh, that's true. I fix it.

Outi’s picture

I changed one more icon in the system.module.css file and fixed the bad url path on the system.admin.css file.

Outi’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Reviewing...

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
174.38 KB

Patch applies well and new SVG icons are loaded. Seems to be a RTBC for me :)

system-svg-icons.png

pameeela’s picture

Looks good to me too!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

echoz’s picture

Status: Fixed » Needs review
FileSize
9.81 KB
10.62 KB
61.14 KB
7.06 KB

Followup!

* These png fallbacks had not been optimized, and now have been run through ImageOptim.

optimize-png.png

* In system.module.css -
The move icon (drag handle) is positioned poorly. This patch improves this and cleans up the related css, including removing a corresponding RTL ruleset where there was no direction specific property/values.

Before

move-icon-before.png

After

move-icon-after.png

* Fixed a place in both system.admin.css and system.module.css where "png" was accidentally "svg".

* In system.admin.css -
Removed LTR comments on icon bkg css that were not direction specific (but hadn't had corresponding RTL css added anyway).

* Corrected one instance of missing RTL css that was legitimately missing and removed an obsolete ruleset.

LewisNyman’s picture

Status: Needs review » Needs work

It looks like the drag icons disappear on hover :(

echoz’s picture

Status: Needs work » Needs review

#13, They did before i patched this. I thought it was intentional because when hovering, the icon is replaced by the "drag handle" cursor. Setting back to needs review to move this along.

Edit: I just checked and this is how the drag handles work on the menu interface, so if it's a bug, it's broader and I believe deserves a new issue.

LewisNyman’s picture

Status: Needs review » Needs work
FileSize
66.14 KB

Ah you're right, I had a look on iPads because I remember doing a fair amount of touch specific styling in #1261002: Draggable tables do not work on touch screen devices.

It needs a little bit of work. I think we can increase the size of the image on touch, which we couldn't do before because it was not SVG.

echoz’s picture

@LewisNyman, so isn't #15 still another issue? I checked on my iPad and the menu admin screen as in your screenshot is identical before and after #12 patch. To be clear, in #11 a patch got committed, and my followup #12 patch is for optimizing the png fallbacks, drag handle positioning (non touch), and css cleanup. Shouldn't this just go through even if it doesn't address an additional issue, nor cause that issue?

LewisNyman’s picture

Status: Needs work » Needs review

Yep my mistake. I'll do one more review when I get time.

LewisNyman’s picture

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

I'm happy with the code fixes here. Thanks @echoz!

LewisNyman’s picture

Issue summary: View changes

Added test pages

alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed ea232c3 and pushed to 8.x. Thanks!

Fixed the following on commit

git pre-commit check failed: file core/misc/icons/787878/key.png should be 644 not 755
git pre-commit check failed: file core/misc/icons/bebebe/key.png should be 644 not 755

Status: Fixed » Closed (fixed)

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