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

core(viewport): create ViewportMeta computed artifact #7264

Merged
merged 13 commits into from
Mar 1, 2019

Conversation

mattzeunert
Copy link
Contributor

Summary

Get the viewport info from a ViewportMeta computed artifact, instead of the font size and tap target audits calling the viewport audit.

Involves making the three audits involved async because computed artifacts are async.

@brendankenny also mentioned including a notApplicable property in the artifact result, for when Lighthouse is run in desktop mode. I've not done that for now, but will consider it when adding the isMobile artifact or adding notApplicable logic to tap targets.

Related Issues/PRs

Closes #7084

@mattzeunert mattzeunert changed the title core(viewport): Create ViewportMeta computed artifact and use that instead of calling Viewport audit core(viewport): create ViewportMeta computed artifact Feb 17, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

just some naming comments

lighthouse-core/test/audits/seo/tap-targets-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/seo/tap-targets-test.js Outdated Show resolved Hide resolved
types/artifacts.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! My comments are really just style/nits.

The computed artifact needs tests, though. I'd imagine most of the computed/viewport-meta-test.js tests will be what's in viewport-test.js now (plus any other coverage you think we need), so hopefully it'll be painless.

lighthouse-core/audits/seo/font-size.js Outdated Show resolved Hide resolved
lighthouse-core/audits/seo/tap-targets.js Outdated Show resolved Hide resolved
types/artifacts.d.ts Outdated Show resolved Hide resolved
lighthouse-core/computed/viewport-meta.js Outdated Show resolved Hide resolved
lighthouse-core/computed/viewport-meta.js Show resolved Hide resolved
lighthouse-core/test/audits/seo/font-size-test.js Outdated Show resolved Hide resolved
lighthouse-core/test/audits/seo/tap-targets-test.js Outdated Show resolved Hide resolved
brendankenny and others added 6 commits February 21, 2019 15:32
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
Co-Authored-By: mattzeunert <matt@mostlystatic.com>
@mattzeunert
Copy link
Contributor Author

mattzeunert commented Feb 21, 2019

Thanks for the feedback @patrickhulce and @brendankenny!

The computed artifact needs tests, though. I'd imagine most of the computed/viewport-meta-test.js tests will be what's in viewport-test.js now (plus any other coverage you think we need), so hopefully it'll be painless.

Moved most of the tests over to viewport-meta-test, what do you think?

@patrickhulce
Copy link
Collaborator

Move most of the tests over to viewport-meta-test, what do you think?

heh, I specifically didn't make the test comment because in refactoring PRs I like to see that the original case and flowthrough works exactly as it used it :)

buuuuuut, since @brendankenny pointed it out, I say yes 👍

@mattzeunert
Copy link
Contributor Author

Move most of the tests over to viewport-meta-test, what do you think?

Missed a d here, have already made the change.

@mattzeunert
Copy link
Contributor Author

@brendankenny What do you think of the PR now?

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM2! 📱🖥🖼📐

@brendankenny brendankenny merged commit 01b09d0 into master Mar 1, 2019
@brendankenny brendankenny deleted the viewport-computed-artifact branch March 1, 2019 21:59
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.

3 participants