Convert Layout Icons from PNG to SVG

[stpaultim]

Description of the need

It has been suggested that we convert layout icons from png to svg, now that we have svg icon library in core. https://github.com/backdrop/backdrop-issues/issues/6480#issuecomment-2081480473

Proposed solution

Convert png icons to svg.

Additional information

[indigoxela]

There might be a bit of a misunderstanding here.

We can easily just use svg instead of png for layout icons (they'd look less blurry and the regions less shifted).

However, as these are included as img tags, there's no way to recolor them. And changing the whole render process to display them in the Layout UI, either as a CSS mask, or inline svg (as markup), might be a breaking change for contrib. As that might require a change in the API. (Existing layouts may provide png not appropriate for CSS mask, and they can never get displayed as inline markup.)

[indigoxela]

Although I'm quite sure, this doesn't meet the intended feature, I provided a PR with SVG instead of PNG.

They look a bit cleaner, even when zooming in far with the browser. That's about it. :wink:

[stpaultim]

@indigoxela - Thanks for the PR. I looked at it and (as you said) there is a noticable improvement in the sharpness of the icons and the already small icon files are now a bit smaller. I like it, but I also don't know if there are any downsides to this.

To be clear, I created this issue as a follow-up issue, but it's not an issue I feel strongly about.

@laryn suggested the following (https://github.com/backdrop/backdrop-issues/issues/6480#issuecomment-2081480473)

I think it could be appropriate to change these to svg, personally. It would fit with all the other svg work being done and make it much easier to recolor these in various themes. (Also, it would make building a representative svg icon for flexible templates much more trivial)

If in fact this does make it easier to build custom icons for flexible layout templates, I would see that as a large benefit. Otherwise, I don't see this is as a very urgent issue.

[indigoxela]

@stpaultim it was exactly that comment by @laryn that gave me the impression, that there must be a misunderstanding somewhere, in how svg as image (in img tag) works. :wink:

Neither is it possible to recolor those by CSS, nor will building custom svg icons per flexi-layout be "trivial". The challenges of such an on-the-fly icon for custom layouts are quite the same, I guess. Be it SVG or raster image (via gd).

[klonos]

Neither is it possible to recolor those by CSS, nor will building custom svg icons per flexi-layout be "trivial".

@indigoxela I have created SVG icons for the core layouts based on lines + currentColor, which allow them to behave the same as the Phosphor icons we use in the admin bar (inherit whatever color has been added via CSS for the text). I've also adjusted them to indicate the main region of each layout with a bit darker shade of the color (using a class within the SVG + CSS opacity, which adapts the darker shade to the color instead of being "hard-coded"):

Please let me know if you'd like a PR against yours, or if you prefer that I created a separate PR as an alternative.

As for automatically-generated icons for flexible layouts, there's already #4104, for which I've created a PR in the past based on native PHP functions like imagecreate()/imagecolorallocate()/imagefilledrectangle()/imagepng() ...it was really close - just needed one specific thing figured out last I recall ...now also needs to be adjusted to be generating SVGs instead of PNGs.

[klonos]

...the svg icon code looks like this:

<svg class="boxton" viewBox="0 0 100 150" xmlns="http://www.w3.org/2000/svg">
  <line class="header" x1="0" y1="7" x2="100" y2="7" />
  <line class="header" x1="0" y1="27" x2="100" y2="27" />
  <line class="main" x1="0" y1="75" x2="100" y2="75" />
  <line class="footer" x1="0" y1="123" x2="100" y2="123" />
  <line class="footer" x1="0" y1="143" x2="100" y2="143" />
  Sorry, your browser does not support inline SVG.
</svg>

...and here's the accompanying CSS (some of these could be added as properties within the SVG file, like stroke="currentColor" for example):

line {
  stroke: currentColor;
}
line.main {
  stroke-opacity: 0.7;
  stroke-width: 70;
}
line.header, line.footer {
  stroke-width: 15;
}
line.special, line.main.special {
  stroke-width: 32;
}

[indigoxela]

@klonos I don't think we can change the core layout icons without considering flexible layouts in the same step, though. (And there are also some contrib layouts.)

Should I close my PR? It was more like a POC, anyway.

[klonos]

I don't think we can change the core layout icons without considering flexible layouts in the same step, though.

I think we should. This change here will be very small (just replace the .png files with .svg ones + some minimal CSS changes), whereas handling automated generation of .svg thumbnails each time a flexible template is saved needs more code/logic into it (and I have a working PR for it in #4104, which I'll loop back to as soon as possible).

And there are also some contrib layouts.

Yes. And some custom layouts as well I am assuming. These can continue to use whatever they want; .png/.jpeg/.gif or switch to .svg. Core should not be responsible for these - same as we are not responsible for the thumbnails shown for contrib themes in the project browser. We should be able to handle it though.

Should I close my PR? It was more like a POC, anyway.

Up to you really. But it does sound like you were experimenting with the idea, so I'm going to unassign the issue from you and assign it to myself, as I would like to get this done. I hope you don't mind.

[klonos]

...perhaps as part of this issue/PR here we can create a .svg file to replace the current .png we are using for flexible templates, but yeah, I don't want to include the automatic generation here.

[indigoxela]

@klonos moved my PR out of the way. :wink:

[klonos]

I've hit a couple of snags:

• 6576

• 6575

I'll keep working on a PR for this, but have added @todo items for the things that need to be addressed properly.

[izmeez]

@klonos Where is your PR? I don't see a link at the top of the issue. Is it just me?

[klonos]

Still working on it @izmeez 🔨 🔧 ...see how in my previous comment I said:

I've hit a couple of snags:

I should have something in the next 24hrs ...by the end of weekend at the very least. So stay tuned! 🙂

[klonos]

I've also added the documentation label to this issue, because I would also like us to outline some basic steps to help people create such icons for their layout templates if they want. It's kinda simple and straight-forward if you start from an existing core-provided icon, and edit it in a text editor (some basic math and playing around might be required).

I was thinking a sub-page here: https://docs.backdropcms.org/documentation/developing-layouts

[klonos]

PR almost ready. ...as an added bonus, layout templates can now add extensionless icon names from the core-provided Phosphor set, and then these will be used. For example, changing the taylor-flipped.info file from this: preview = taylor-flipped.svg ...to this: preview = lego

Has the following effect:

That should help contrib/custom layout templates 😉

All flexible layout templates are currently using this new "trick" to set their icon to pencil-ruler. I randomly chose that icon for now, but no need to waste time bikeshedding about it, since that will be short-lived (planning to work on #4104 next).

[klonos]

Here's the PR: https://github.com/backdrop/backdrop/pull/4769

For those that want to review smaller sets of changes, I've kept a separate commit for the deletions of the .png files and the addition of the respective .svg files. The commit that has the rest of the changes is this one: backdrop/backdrop@b0f2a8a (#4769)

[klonos]

...one set of tests is failing for php5 and php7, but I need to take a break. Sorry.

[klonos]

OK, tests fixed (another snag, related to our SVG support implementation and php8 specifically - see #6577 ), however I just noticed that in the PR sandbox the icons are not being generated/loaded 😞 ...my local dev is based on DDEV running php 8.3.6, whereas the sandbox is running 8.2.11 - both are on Apache/2.4.57 (Debian).

More digging required I guess. That's sad, because this is looking kinda awesome on my local.

Still setting this to needs code review and needs testing for people that can pull the PR and test things locally.

[jenlampton]

First @klonos - I love this change! Thank you for opening this :)

The PR looks great, but I have concerns about styling the Rolph layout to indicate a <main> region when it doesn't have one. I think that's a bit deceiving. If the layout doesn't have one, the image should reflect that so there are no surprises down the road :)

[klonos]

@jenlampton yes, I agree. I'll work to address the feedback from the dev meting just then, including that problem with Rolph. ...what you need to check out though is this: #6461

...what I meant to say is that Rolph actually does provide a default region. See: https://github.com/backdrop/backdrop/blob/1.x/core/layouts/rolph/rolph.info#L17 (so I should really fix that svg to make the first column the main one, rather than removing it entirely from it)

[jenlampton]

@klonos oh, I thought you were indicating a <main> region, but you were instead indicating a default region -- that makes more sense actually. And since defined in the layout's info file it's also much easier to determine :D

[klonos]

This is currently blocked on the following issues:

• [x] #6577
• [x] #6576
• [ ] #6575

I'm trying to unblock all these for the 1.28.1 release if possible.

[klonos]

One blocker down - two more to go 🙂 ...I've updated the PR to revert the changes that have now been merged into core as part of #6577.

[klonos]

...another blocker gone - only one left 🙂 ...updated the PR again, to remove the changes that are now in core with #6576, and rebased it as well while at it.