Conversation
4 tasks
Member
|
Would be cool to get tests in first so that we we can have more confidence in not regressing anything: #17 |
Contributor
Author
|
#18 Covers this and includes the necessary PHP changes as well, so let's go with that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We'd like to use mShots to dynamically generate preview screenshots in
wordpress.com/newand it's looking great in general but there are a few themes that look pretty odd (see Automattic/wp-calypso#43428).This PR is to discuss enabling more parameters for mshots to solve those problems.
I'm just dumping exploratory code here, but if I can get some agreement that this is the right way to go I'll tidy this up and extend the changes up to the API.
I'm working on OSX, so I don't have the kernel extensions required to run the mshots service, so I've just been running the snapshot lib directly. After cloning mshots into
/opt/localand runningnpm installas per the instructions, I've been testing using:This takes a moment before it dumps the reply object, and then you can
open /opt/mshots/test.jpgto see the image.In particular, I've been looking at the Rockfield theme. It's got that cool effect where the background images change as the text wipes through them:
Unfortunately, the theme gets a bit confused when the viewport is big enough to get an impression of the theme:
snapshot.add_to_queue( { width: 1200, height: 3072, url: test_url, file: 'test.jpg', })Each picture is being scaled to fill the entire scrollHeight, instead of the smaller viewport we'd see while scrolling through, so the first picture ends up being a picture of a giant hand

Now, there is an argument to be made that we could/should fix the css, but it's not really just another screen size to push on designers because we want different behaviour for this one, so I spent some time seeing if I could persuade puppeteer to play nice, and I found the clip parameter:
snapshot.add_to_queue( { width: 1200, height: 800, url: test_url, file: 'test.jpg', clip: { x:0, y:0, width:1200, height:3072 } } )You can see that this is not perfect, as there's no image behind "Discover Our Menu", but it seems much better to me in this specific instance, and much more intuitive in general - we can keep the viewport parameters in typical sane ranges and use the clip parameters to "scroll down" instead.
I also checked out the 'fullPage' option, but it wasn't what I needed, but I saw someone ask for it so I thought I'd include it here (fullPage overrides both viewport and clip):
snapshot.add_to_queue( { fullPage: true, url: test_url + '&viewport_height=700', file: 'test.jpg' } )Note that here we need the preview viewport hack on the website itself to get halfway reasonable result, otherwise the whole picture is just the giant hand with a little text at the bottom. I think the fact that we don't need that hack if we use the clip parameters instead is a really good sign that it's the right way to go.