-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(meta): refactor all meta audits to single artifact #7025
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -199,7 +199,7 @@ class FontSize extends Audit { | |
title: str_(UIStrings.title), | ||
failureTitle: str_(UIStrings.failureTitle), | ||
description: str_(UIStrings.description), | ||
requiredArtifacts: ['FontSize', 'URL', 'Viewport'], | ||
requiredArtifacts: ['FontSize', 'URL', 'MetaElements'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no changes to functionality needed here because it just calls out to the viewport audit |
||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ class ThemedOmnibox extends MultiCheckAudit { | |
failureTitle: 'Does not set an address-bar theme color', | ||
description: 'The browser address bar can be themed to match your site. ' + | ||
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/address-bar).', | ||
requiredArtifacts: ['Manifest', 'ThemeColor'], | ||
requiredArtifacts: ['Manifest', 'MetaElements'], | ||
}; | ||
} | ||
|
||
|
@@ -43,13 +43,13 @@ class ThemedOmnibox extends MultiCheckAudit { | |
} | ||
|
||
/** | ||
* @param {LH.Artifacts['ThemeColor']} themeColorMeta | ||
* @param {LH.Artifacts['MetaElements'][0]|undefined} themeColorMeta | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we make an alias of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the only usage of it, but sure :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ha, whoops |
||
* @param {Array<string>} failures | ||
*/ | ||
static assessMetaThemecolor(themeColorMeta, failures) { | ||
if (themeColorMeta === null) { | ||
if (!themeColorMeta) { | ||
failures.push('No `<meta name="theme-color">` tag found'); | ||
} else if (!ThemedOmnibox.isValidColor(themeColorMeta)) { | ||
} else if (!ThemedOmnibox.isValidColor(themeColorMeta.content || '')) { | ||
failures.push('The theme-color meta tag did not contain a valid CSS color'); | ||
} | ||
} | ||
|
@@ -79,14 +79,15 @@ class ThemedOmnibox extends MultiCheckAudit { | |
/** @type {Array<string>} */ | ||
const failures = []; | ||
|
||
const themeColorMeta = artifacts.MetaElements.find(meta => meta.name === 'theme-color'); | ||
const manifestValues = await ManifestValues.request(artifacts.Manifest, context); | ||
ThemedOmnibox.assessManifest(manifestValues, failures); | ||
ThemedOmnibox.assessMetaThemecolor(artifacts.ThemeColor, failures); | ||
ThemedOmnibox.assessMetaThemecolor(themeColorMeta, failures); | ||
|
||
return { | ||
failures, | ||
manifestValues, | ||
themeColor: artifacts.ThemeColor, | ||
themeColor: (themeColorMeta && themeColorMeta.content) || null, | ||
}; | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
/** | ||
* @license Copyright 2018 Google Inc. All Rights Reserved. | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
*/ | ||
'use strict'; | ||
|
||
const Gatherer = require('./gatherer.js'); | ||
const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len | ||
|
||
class MetaElements extends Gatherer { | ||
/** | ||
* @param {LH.Gatherer.PassContext} passContext | ||
* @return {Promise<LH.Artifacts['MetaElements']>} | ||
*/ | ||
async afterPass(passContext) { | ||
const driver = passContext.driver; | ||
|
||
// We'll use evaluateAsync because the `node.getAttribute` method doesn't actually normalize | ||
// the values like access from JavaScript does. | ||
return driver.evaluateAsync(`(() => { | ||
${getElementsInDocumentString}; | ||
|
||
return getElementsInDocument('head meta').map(meta => { | ||
return { | ||
name: meta.name.toLowerCase(), | ||
content: meta.content, | ||
}; | ||
}); | ||
})()`, {useIsolation: true}); | ||
} | ||
} | ||
|
||
module.exports = MetaElements; |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,12 @@ const validViewport = 'width=device-width'; | |
/* eslint-env jest */ | ||
|
||
describe('SEO: Font size audit', () => { | ||
const makeMetaElements = viewport => viewport ? [{name: 'viewport', content: viewport}] : []; | ||
|
||
it('fails when viewport is not set', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: null, | ||
MetaElements: makeMetaElements(null), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the conditional in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, done |
||
FontSize: [], | ||
}; | ||
|
||
|
@@ -29,7 +31,7 @@ describe('SEO: Font size audit', () => { | |
it('fails when less than 60% of text is legible', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 100, | ||
visitedTextLength: 100, | ||
|
@@ -51,7 +53,7 @@ describe('SEO: Font size audit', () => { | |
it('passes when there is no text', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 0, | ||
visitedTextLength: 0, | ||
|
@@ -70,7 +72,7 @@ describe('SEO: Font size audit', () => { | |
it('passes when more than 60% of text is legible', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 330, | ||
visitedTextLength: 330, | ||
|
@@ -106,7 +108,7 @@ describe('SEO: Font size audit', () => { | |
}; | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 7, | ||
visitedTextLength: 7, | ||
|
@@ -130,7 +132,7 @@ describe('SEO: Font size audit', () => { | |
it('adds a category for failing text that wasn\'t analyzed', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 100, | ||
visitedTextLength: 100, | ||
|
@@ -152,7 +154,7 @@ describe('SEO: Font size audit', () => { | |
it('informs user if audit haven\'t covered all text on the page', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 100, | ||
visitedTextLength: 50, | ||
|
@@ -172,7 +174,7 @@ describe('SEO: Font size audit', () => { | |
it('maintains 2 trailing decimal places', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 323, | ||
visitedTextLength: 323, | ||
|
@@ -191,7 +193,7 @@ describe('SEO: Font size audit', () => { | |
it('maintains 2 trailing decimal places with only 1 leading digit', () => { | ||
const artifacts = { | ||
URL, | ||
Viewport: validViewport, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
totalTextLength: 323, | ||
visitedTextLength: 323, | ||
|
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.
document the reason for this?
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.
done