-
Notifications
You must be signed in to change notification settings - Fork 6
Allow specifying the viewport dimensions when running benchmark-web-vitals #164
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
Conversation
felixarntz
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.
@westonruter Awesome, I can't believe we didn't have something like this before - so were always only testing with this "small desktop"-ish viewport?! Wow.
One recommendation for enabling simpler usage rather than having to specify specific widths, but otherwise this is good.
| showVariance: Boolean( opt.showVariance ), | ||
| cpuThrottleFactor: null, | ||
| networkConditions: null, | ||
| windowViewport: { width: 960, height: 700 }, // Viewport similar to @wordpress/e2e-test-utils 'large' configuration. |
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.
It makes sense to have this parameter, but I wonder whether we should additionally have more than just this one default, to simplify usage.
I'm thinking about our typical mobile vs desktop differentiation. Maybe we could allow passing special string values mobile and desktop for the --window-viewport argument, and in that case rely on a specific default for that viewport type that we set?
I wonder what PageSpeed Insights uses, maybe we could align with that?
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.
Good idea. That did cross my mind, but I didn't implement it. I picked the 360x800 and 1920x1080 since they are the most common mobile and desktop dimensions currently. In 1f3b1c5 I've implemented the mobile and desktop aliases, and I've set their default values to correspond to what Lighthouse uses.
|
Something else to consider, beyond adding support for |
|
Thinking about this some more, I think we should also have a |
felixarntz
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 @westonruter, LGTM!
|
@westonruter Those ideas sound good to me, we could explore them in separate PRs. Though this could also wait until we find an actual use-case that would require us to support those. |
|
As it stands right now, specifying |
|
Follow up: #166 |
The
benchmark-web-vitalscommand has only been testing a desktop-ish sized viewport at 960x700. In order to test a mobile viewport or a more common desktop viewport, this PR introduces a--window-viewport(-w) parameter which allows the desired window dimensions to be supplied like360x800. The default remains960x700.