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

Higher Res Images #2

Merged
merged 1 commit into from
Sep 14, 2015
Merged

Higher Res Images #2

merged 1 commit into from
Sep 14, 2015

Conversation

mushishi78
Copy link

With the new responsive break points, the 250px width gets stretched to fit on phones up to 600px wide. As such, I thought it'd be nice to have a higher res images so I used a tool to batch grab as many screenshots as I could. While I was at it I converted all the images to jpg and compressed them, to keep the image load time down.

As some of the images are still 250x200 and some of the screengrabs may not be as the author intended, I thought I'd make a separate branch and pull request to review the images on.

@jekyllrc/themes

@gynter
Copy link
Member

gynter commented Sep 7, 2015

Whoa, nice. Did You do that all manually? If not, then maybe commit the scripts what You used to the repo (i.e _scripts/).

@mushishi78
Copy link
Author

Well I did the screengrabs with a firefox addon, Grab Them All, the resizing with Imagemagick and the compression with Riot plugin for Gimp. So not scriptable, but perhaps writing a little script would be a good idea.

@gynter
Copy link
Member

gynter commented Sep 7, 2015

The only issue with that is we have to start generating thumbnails from high resolution images, because resizing those with HTML is not an option. It's performance and bandwith killer, especially for mobile users.

@mushishi78
Copy link
Author

Okay I pretty much got all the images now. I left the animated gif someone made, because they must have gone to trouble to do that.

@gynter I don't own a smartphone, so I can't speak to the performance resizing images, but it seems alright on my computer. If people start adding unoptimised images, it's not too much trouble to optimise a batch of images every now and then. And if performance becomes a problem, perhaps lazy loading images would be an idea.

I'll wait on anyone else input before merging this in, but I'm not planning on making any more changes though.

@gynter
Copy link
Member

gynter commented Sep 8, 2015

Sry, wrong button :)

Don't merge this yet, it contains images which are with wrong aspect ratio (stretched out), for example the Book and Dr Jekyll's Bootstrap theme thumbnails.

Also, I don't consider those images as high res anymore, since they are compressed so much that they look all fuzzy and ugly.

I'm not against high-res images, not at all, but then we should generate proper images, high ress, less compression, very good quality and lossy thumbnails.

@gynter gynter closed this Sep 8, 2015
@gynter gynter reopened this Sep 8, 2015
@jacobtomlinson
Copy link

I agree that we should be generating high res images but also low res and thumbnail ones as well and then loading in different ones depending on device.

@jacobtomlinson
Copy link

We should also think about pagination or loading images on scroll to keep the initial bandwidth down. I used a nice JavaScript library a while back which doesn't load images if they are off the page until you scroll and they get to 200px below the bottom. Most users don't see the images load and it cuts bandwidth massively.

@mushishi78
Copy link
Author

I moved the grab image script to the main branch, and squashed the commits on this one to make it simpler.

The new images on this branch are only 1MB bigger than the main branch because of the compression. But I don't know much about compression and quality. Would it be helpful to put up the raw images instead?

The new images are all three times the original dimensions, 750x600. We could compromise and use twice the dimensions, 500x400. This could be good a temporary measure until something better is worked out.

@mushishi78
Copy link
Author

I've added lazy loading images using echo.js to the main branch.

@gynter
Copy link
Member

gynter commented Sep 8, 2015

Very nice!

@mushishi78 Please also look into those screenshots with illegal aspect ratio (stretched out) as I mentioned before:

Don't merge this yet, it contains images which are with wrong aspect ratio (stretched out), for example the Book and Dr Jekyll's Bootstrap theme thumbnails.

@mushishi78
Copy link
Author

I've rebased this up to date with main branch and added new images for the newer posts.

@gynter could you test whether the load times for these images are acceptable with the lazy loading. I'm mainly waiting for your approval/input on this branch, as I'm completely fine with merging this in as is.

@gynter
Copy link
Member

gynter commented Sep 14, 2015

Please re-check all images. There are several ones with wrong/stretched out aspect ratio, for example: Uno-dbyll, Compass, Strange Case, Landing Page, Jekyll Metro, Noita, Otter Pop, Lagom, and probably more, please re-check all of em visually. Also some of those are fine on jekyllthemes.org.

If You get the stretched images fixed, then feel free to merge this to master.

Note: don't forget to remove the unnecessary branches like demos-dropdown and after merging higher-res-images from Github too.

@mushishi78
Copy link
Author

Thanks for a list of images, it's too many images not to have a second set of eyes. And good point about the leftover branches.

@mushishi78
Copy link
Author

I've replaced the one you named. I accept that there are probably still ones that are stretched or wrong, but I'm going to go ahead and merge this and correct them as they get noticed.

mushishi78 added a commit that referenced this pull request Sep 14, 2015
@mushishi78 mushishi78 merged commit d6ba06c into gh-pages Sep 14, 2015
@mushishi78 mushishi78 deleted the higher-res-images branch September 14, 2015 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants