core(audit): align meta properties with LHR #5540

patrickhulce · 2018-06-21T17:02:30Z

We updated the output, but not the input. Before we get stuck with helpText for a year and officially ship the major, we should do that :)

name -> id
description -> title
helpText -> description

paulirish

hot damn.

looking around.. computed artifacts and gatherers still use a 'name', but thats a separate issue.

It's a little challenging to determine if any descriptions were left out of the rename from here. :/ But i'm sure it's fine. :)

curious: how much of this was done via 'rename symbol' ?

patrickhulce · 2018-06-21T17:25:05Z

It's a little challenging to determine if any descriptions were left out of the rename from here. :/ But i'm sure it's fine. :)

Yeah it was a little tricky renaming to something else we were using, haha.

curious: how much of this was done via 'rename symbol' ?

0% :(, it doesn't seem to be very reliable with js yet, maybe it's an insiders bug, but first time I tried it, it went nuts and changed an extra ~70 unnecessary files with imports and other nonsense.

type checking caught a few extra things though which is nice!

paulirish · 2018-06-21T17:31:47Z

there's two more instances of helptext that should probably be renamed (though its not a bug). also some FailureDesc

patrickhulce · 2018-06-21T17:54:25Z

hm @paulirish where? vscode isn't finding any more non-string ones :/

brendankenny

nice clean up! Mostly comments to try to ward off the // eslint-disable-next-line max-len invasion :)

brendankenny · 2018-06-21T22:29:20Z

lighthouse-core/audits/accessibility/th-has-data-cells.js

          '`[role="columnheader"/"rowheader"]` do not have data cells they describe.',
-      helpText: 'Screen readers have features to make navigating tables easier. Ensuring table ' +
+      // eslint-disable-next-line max-len


other shifts were fixed, just fix this one as well? :)

brendankenny · 2018-06-21T22:30:22Z

lighthouse-core/audits/byte-efficiency/uses-text-compression.js

-      helpText: 'Text-based responses should be served with compression (gzip, deflate or brotli)' +
+      title: 'Enable text compression',
+      // eslint-disable-next-line max-len
+      description: 'Text-based responses should be served with compression (gzip, deflate or brotli)' +


easy shift :)

brendankenny · 2018-06-21T22:31:17Z

lighthouse-core/audits/first-contentful-paint.js

+      id: 'first-contentful-paint',
+      title: 'First Contentful Paint',
+      // eslint-disable-next-line max-len
+      description: 'First contentful paint marks the time at which the first text/image is painted. ' +


😬😬🎉

brendankenny · 2018-06-21T22:31:55Z

lighthouse-core/audits/first-meaningful-paint.js

-      helpText: 'First Meaningful Paint measures when the primary content of a page is visible. ' +
+      id: 'first-meaningful-paint',
+      title: 'First Meaningful Paint',
+      // eslint-disable-next-line max-len


🌵 🍰 😰

brendankenny · 2018-06-21T22:32:33Z

lighthouse-core/audits/manual/pwa-page-transitions.js

-      name: 'pwa-page-transitions',
-      helpText: 'Transitions should feel snappy as you tap around, even on a slow network, a key ' +
+      id: 'pwa-page-transitions',
+      // eslint-disable-next-line max-len


🏷 🍃 🚈

brendankenny · 2018-06-21T22:39:24Z

lighthouse-core/test/fixtures/invalid-audits/missing-description.js

-      name: 'missing-description',
-      helpText: 'This is missing required description (and failure description)',
+      id: 'missing-description',
+      title: 'Missing required artifacts',


description

err.. if its the same test then we'd leave out title and failureTitle but provide a description.

Ha, I meant that the title said it was missing artifacts but good point

we already have missing-title for that, this one is really the help text check, git diff is confused

brendankenny · 2018-06-21T22:39:40Z

lighthouse-core/test/fixtures/invalid-audits/missing-audit.js

-      name: 'missing-category',
-      description: 'Missing required artifacts',
-      helpText: 'This is missing required artifacts',
+      id: 'missing-category',


we also got these wrong :)

brendankenny · 2018-06-21T22:42:18Z

docs/architecture.md

-* **Audit failure description** - Short user-visible title for a failing  audit. eg. “Some image elements do not have `[alt]` attributes.”
-* **Audit help text** - Explanation of why the user should care about the audit. Not necessarily how to fix it, unless there is no external link that explains it. ([See helpText guidelines](CONTRIBUTING.md#helptext-guidelines)). eg. “Informative elements should aim for short, descriptive alternate text. Decorative elements can be ignored with an empty alt attribute. [Learn more].”
+* **Audit title** - Short user-visible title for the successful audit. eg. “All image elements have `[alt]` attributes.”
+* **Audit failure title** - Short user-visible title for a failing  audit. eg. “Some image elements do not have `[alt]` attributes.”


should we change it to **Audit failureTitle** just to be crystal clear it's that property name?

brendankenny · 2018-06-21T22:43:35Z

lighthouse-core/config/config.js

@@ -8,13 +8,13 @@
 const defaultConfigPath = './default-config.js';
 const defaultConfig = require('./default-config.js');
 const fullConfig = require('./full-config.js');
-const constants = require('./constants');
+const constants = require('./constants.js');


brendankenny · 2018-06-21T22:44:26Z

lighthouse-core/test/config/config-test.js

@@ -8,7 +8,7 @@
 const Config = require('../../config/config');
 const assert = require('assert');
 const path = require('path');
-const defaultConfig = require('../../config/default-config.js');
+const defaultConfig = require('../../config/default-config');


lol this was also vscode, I have no idea why it's inconsistent

oooooh wait I think it's trying to match the ones around it! so clever :D

paulirish · 2018-06-22T11:21:44Z

lighthouse-core/test/fixtures/invalid-audits/missing-description.js

-      name: 'missing-description',
-      helpText: 'This is missing required description (and failure description)',
+      id: 'missing-description',
+      title: 'Missing required artifacts',


err.. if its the same test then we'd leave out title and failureTitle but provide a description.

paulirish · 2018-06-22T11:23:34Z

lighthouse-core/test/config/config-test.js

-      audits: [basePath + '/missing-help-text'],
-    }), /meta.helpText property/);
+      audits: [basePath + '/missing-description'],
+    }), /meta.description property/);

    assert.throws(_ => new Config({
      audits: [
        class EmptyStringHelpTextAudit extends Audit {


hm @paulirish where? vscode isn't finding any more non-string ones :/

yeah i was just looking for any of them. relax that case sensitivity ;)

helptext -> description

paulirish · 2018-06-22T11:23:39Z

lighthouse-core/test/config/config-test.js

-              name: 'empty-string-help-text',
-              description: 'description',
-              helpText: '',
+              id: 'empty-string-help-text',


helptext -> description

and you fixed the one in valid-custom-audit already. :)

oh gotcha :) fixed!

paulirish · 2018-06-22T11:26:43Z

lighthouse-core/test/config/config-test.js

-      audits: [basePath + '/missing-description'],
-    }), /meta.description property/);
+      audits: [basePath + '/missing-title'],
+    }), /meta.title property/);

    assert.throws(_ => new Config({
      audits: [
        class BinaryButNoFailureDescAudit extends Audit {


failureTitle

core(audit): align meta properties with LHR

debd975

patrickhulce requested review from brendankenny, paulirish and vinamratasingal-zz as code owners June 21, 2018 17:02

paulirish approved these changes Jun 21, 2018

View reviewed changes

Merge branch 'master' into audit_property_names

124d69a

minor cleanup

c4e73ec

brendankenny approved these changes Jun 21, 2018

View reviewed changes

paulirish reviewed Jun 22, 2018

View reviewed changes

feedback

64a9925

patrickhulce merged commit 6285e1e into master Jun 25, 2018

patrickhulce deleted the audit_property_names branch June 25, 2018 16:14

paulirish mentioned this pull request Jun 26, 2018

core: [minor] migrate remaining .description -> .title #5566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(audit): align meta properties with LHR #5540

core(audit): align meta properties with LHR #5540

patrickhulce commented Jun 21, 2018

paulirish left a comment

patrickhulce commented Jun 21, 2018

paulirish commented Jun 21, 2018 •

edited

Loading

patrickhulce commented Jun 21, 2018

brendankenny left a comment

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

paulirish Jun 22, 2018

brendankenny Jun 22, 2018

patrickhulce Jun 22, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

patrickhulce Jun 22, 2018

brendankenny Jun 21, 2018

brendankenny Jun 21, 2018

patrickhulce Jun 22, 2018

patrickhulce Jun 22, 2018

paulirish Jun 22, 2018

paulirish Jun 22, 2018

patrickhulce Jun 22, 2018

paulirish Jun 22, 2018

patrickhulce Jun 22, 2018

paulirish Jun 22, 2018

patrickhulce Jun 22, 2018

core(audit): align meta properties with LHR #5540

core(audit): align meta properties with LHR #5540

Conversation

patrickhulce commented Jun 21, 2018

paulirish left a comment

Choose a reason for hiding this comment

patrickhulce commented Jun 21, 2018

paulirish commented Jun 21, 2018 • edited Loading

patrickhulce commented Jun 21, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jun 21, 2018 •

edited

Loading