Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview image from assets folder doesn’t seem to work with svg #2746

Closed
mrflix opened this issue Jul 28, 2020 · 10 comments
Closed

Preview image from assets folder doesn’t seem to work with svg #2746

mrflix opened this issue Jul 28, 2020 · 10 comments
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@mrflix
Copy link
Contributor

mrflix commented Jul 28, 2020

Describe the bug
The pages#Preview images example loading an image from the asset folder doesn’t seem to work with svg images.
It results in panel error message:

The section “sectionname” could not be loaded: The file::version component must return a File or FileVersion object

To Reproduce
I reproduced it using a fresh starterkit:

  1. put an svg in assets/images/test.svg
  2. load that svg in model/album.php:
class AlbumPage extends Page
{
    public function cover()
    {
        return new Asset('assets/images/test.svg');
    }
}
  1. The error message appears in the panel dashboard.

Expected behavior
The page list would render with the test.svg as the preview image.

Screenshots
Error message:
image

Kirby Version
3.4.0

Console output

{"status":"error","message":"The file::version component must return a File or FileVersion object","code":400,"exception":"Kirby\Exception\InvalidArgumentException","key":"error.invalidArgument","file":"mono-besteck/kirby/src/Cms/FileModifications.php","line":195,"details":[],"route":"site/sections/([a-zA-Z0-9\.\-_%= \+\@\(\)]+)"}

Context
I'm creating page preview svgs on the fly. Why?

  1. My preview images are brand logos and they stick to the sides of the preview area so I add padding via svg.
  2. Each brand has its own background-color and since I can't set the background-color via back: "{{ page.background }}", I add it in the dynamically created svg. My previewImage function looks like this:
public function previewImage()
{	
	$filename = 'assets/images/previews/'. $this->uid() .'.svg';
	file_put_contents($filename, '<svg width="100" height="100" viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"><rect width="100" height="100" fill="'. $this->background()->value() .'" /><image x="10" y="10" width="80" height="80" xlink:href="'. $this->logo()->url() .'" /></svg>');
	return new Asset($filename);
}
@afbora afbora added the type: bug 🐛 Is a bug; fixes a bug label Jul 28, 2020
@afbora
Copy link
Member

afbora commented Jul 28, 2020

I guess we need check image is resizable before thumb action in panelImage() method, something like that:

if ($image->isResizable() === true) {
    // thumb action
}

@afbora afbora added this to the 3.4.2 milestone Jul 30, 2020
@distantnative
Copy link
Member

@afbora I think checking for resizable will prevent the error but only with the result that no panel preview image is shown. What @mrflix of course wants, is that the SVG file is shown as preview. So we need to add SVG support. Which will be trickier cause an SVG needs to be handled quite a bit differently (e.g. not via srcset)...

@afbora
Copy link
Member

afbora commented Jul 31, 2020

I guess we need check image is resizable before thumb action in panelImage() method, something like that:

if ($image->isResizable() === true) {
    // thumb action
}

@distantnative I had tested this and worked as expected, am I missing something? 🤔

@mrflix
Copy link
Contributor Author

mrflix commented Jul 31, 2020

Adding an image.padding option and query support to image.back would solve my needs as well but I guess those additions would be too specific.

Regarding SVG support: the previews already support svg. When you upload a svg image it shows the preview in the list/grid. It has an unneccesary srcset attribute, but that's not a problem.

@distantnative
Copy link
Member

I have been looking in the wrong places then 😅 withdrawing my comments.

@mrflix we have been playing around with an implementation that also supports hex codes as value for back - query support wouldn't be too hard to add as well, I guess. But I can't promise when those changes will make it into a release.
padding sounds too much of a niche use case. But I think if we could somehow treat SVGs like icons/emojis and center them, that would help your case as well - did I understand that correctly?

@afbora
Copy link
Member

afbora commented Jul 31, 2020

@distantnative Should we show vector images directly or do we need to modify them with viewbox?

@mrflix
Copy link
Contributor Author

mrflix commented Jul 31, 2020

@mrflix we have been playing around with an implementation that also supports hex codes as value for back - query support wouldn't be too hard to add as well, I guess. But I can't promise when those changes will make it into a release.

Its a niche case aswell but the more popular one probably. Custom back color would be nice to have.

padding sounds too much of a niche use case. But I think if we could somehow treat SVGs like icons/emojis and center them, that would help your case as well - did I understand that correctly?

It's already getting centered. The logos are just sticking to the sides, so it doesn't look good. Actually now that I think of it I had this situation already several times when working with logos in the panel: the preview always looks bad because they stick to the sides. Padding would be a really nice option actually.

@afbora
Copy link
Member

afbora commented Aug 13, 2020

@distantnative I'll try take care of this issue. Have a question for you. Can't we just show vector images directly without any modification like widh/height/viewBox etc..?

@bastianallgeier
Copy link
Member

When we use an class name check it's actually a lot simpler. I just committed a small fix.

@mrflix
Copy link
Contributor Author

mrflix commented Sep 7, 2020

Thank you Ahmet, Nico and Bastian!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

No branches or pull requests

4 participants