-
Notifications
You must be signed in to change notification settings - Fork 381
Update Missing Cover Image #4876
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
base: dev
Are you sure you want to change the base?
Conversation
new cover image that is smaller (webp) and with a light gray background to make theme development easier.
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @crhallberg. A few questions/comments:
1.) You need to run composer fix to tweak some style issues.
2.) There's at least one hard-coded reference to the old cover image here, which we might want to update to match:
| <img src="<?=$this->imageLink('noCover2.gif')?>" class="olSubjectImage" alt="<?=$this->escapeHtmlAttr($work['title'])?>"> |
3.) There are likely some tests that need tweaking too.
4.) Why webp format? I don't mind adding support for the format, but I wonder if it would be wiser to use a less exotic format for this particular image in the interest of broad compatibility. (At least, GitHub itself seems to be really struggling to deal with this file in its own files tab, which makes me wonder if that's indicative of broader support elsewhere).
|
I picked webp to make the image as small as possible. It is a modern format but is widely supported. |
|
I checked the tests and they are all setting the config for the test, so they are all good. |
|
Thanks, @crhallberg. It occurred to me that the hard-coded no-cover image in OpenLibrarySubjects is not really a good idea, which led me to open #4926 to straighten it out. Do you mind taking a look at that? I think it might be a good idea to get that merged first, and then we can revisit this issue with one fewer file in the PR. |
|
I believe that at this point it should be safe to use webp. The size won't make much difference though, since it's only loaded once and cached, right? And the new image looks much better than the old one. I realize that this is configurable, but there are still a couple of things that worry me a bit:
|
I propose switching to a new cover image for development ease. An image with a light gray background will make the spacing and alignment of elements easier than the current white on white. I was hoping to reduce the file size but it's hard to beat a 368 byte GIF! I can do better if this is a concern.
TODO
Preview
The new image was adapted from this image listed under the permissive Pixabay Content License - which allows for adaptation and intergrated commercial use without attribution. That being said, thank you Memed_Nurrohmad for your image!