Report: lingo cleanup, visual cleanup, remove redundant messaging #1598

ebidel · 2017-02-01T19:17:42Z

R: all

Users get confused when an audit description says "Site does not use x API..." and there's a red "X" for the audit result. "X" means the audit failed, but the double negative is confusing. I've heard the complaint a few times in the last couple of days and decided to fix it.

This PR changes the lingo we use in the report to be consistent throughout:

Replace "Site does not use X" -> "Avoids X"
Replace "Site uses..." -> "Page uses..." (we're testing a page and not the entire site)
Remove redundant debugStrings like this:

Removes categories from the audit names. Reduces visual clutter.

Before:

Now:

…dits

ebidel · 2017-02-01T19:18:31Z

lighthouse-core/report/templates/report-template.html

@@ -153,9 +153,9 @@ <h2 class="config-section__title">Blocked URL Patterns</h2>
                <li class="subitem {{#if subItem.comingSoon}}--coming-soon{{/if}} {{#if (shouldShowHelpText subItem.score)}}--show-help{{/if}}">

                  <p class="subitem__desc">
-                    {{#unless ../../scored }}
+                    <!--{{#unless ../../scored }}


I left this in for now in case we want to bring it back some day. Also left the category in each audit.

leaving category in is fine, I'm sure #1463 will have something to say about it anyways, but I'd say just delete this. We definitely don't want.

ebidel · 2017-02-01T19:18:50Z

cc @kaycebasques

brendankenny

Big improvement. I personally find the word choice a lot clearer in correspondence between ✘/✓ and audit description and avoiding double negatives, but it would be good to get everyone else's opinion on the change.

brendankenny · 2017-02-01T19:52:37Z

lighthouse-core/audits/manifest-display.js

@@ -57,7 +57,7 @@ class ManifestDisplay extends Audit {
      rawValue: hasRecommendedValue,
      displayValue
    };
-    if (!hasRecommendedValue) {
+    if (manifest && !hasRecommendedValue) {


I can't see this in your screenshot, but don't we want to keep this check so that if there wasn't a manifest it won't display Manifest display property should be set?

This new check will only show the "Manifest display property should be set" debug string if there is a manifest and the display property is missing. Here's what it looks like when a manifest is missing:

oh, my brain is broken. Somehow I thought the diff was the reverse.

brendankenny · 2017-02-01T19:54:01Z

lighthouse-core/audits/manifest-icons-min-144.js

@@ -42,8 +42,7 @@ class ManifestIconsMin144 extends Audit {

    if (icons.doExist(manifest) === false) {
      return ManifestIconsMin144.generateAuditResult({
-        rawValue: false,
-        debugString: 'WARNING: No icons found in the manifest'
+        rawValue: false


maybe remove the giant WARNING from the debugString below while you're fixing these?

brendankenny · 2017-02-01T19:54:19Z

lighthouse-core/audits/manifest-icons-min-192.js

@@ -45,8 +45,7 @@ class ManifestIconsMin192 extends Audit {

    if (icons.doExist(manifest) === false) {
      return ManifestIconsMin192.generateAuditResult({
-        rawValue: false,
-        debugString: 'WARNING: No icons found in the manifest'
+        rawValue: false


same thing with WARNING below

brendankenny · 2017-02-01T19:58:47Z

lighthouse-core/audits/unused-css-rules.js

      helpText: 'Remove unused rules from stylesheets to reduce unnecessary ' +
-          'bytes consumed by network activity. [Learn more](https://developers.google.com/speed/docs/insights/OptimizeCSSDelivery)',
+          'bytes consumed by network activity. ' +


why did this line change?

it was over 100 chars

brendankenny · 2017-02-01T19:59:40Z

lighthouse-core/audits/viewport.js

@@ -27,7 +27,7 @@ class Viewport extends Audit {
    return {
      category: 'Mobile Friendly',
      name: 'viewport',
-      description: 'HTML has a `<meta name="viewport">` tag containing `width` or `initial-scale`',
+      description: 'Page contains a `<meta name="viewport">` tag with `width` or `initial-scale`',


if switching from HTML to Page that means tag should become element :)

brendankenny · 2017-02-01T20:01:05Z

lighthouse-core/gather/gatherers/manifest.js

@@ -45,10 +45,6 @@ class Manifest extends Gatherer {
          let errorString;
          if (response.url) {
            errorString = `Unable to retrieve manifest at ${response.url}`;
-          } else {
-            // The driver will return an empty string for url and the data if


maybe retain comment above if (response.url) {?

brendankenny · 2017-02-01T20:05:32Z

lighthouse-core/report/templates/report-template.html

@@ -153,9 +153,9 @@ <h2 class="config-section__title">Blocked URL Patterns</h2>
                <li class="subitem {{#if subItem.comingSoon}}--coming-soon{{/if}} {{#if (shouldShowHelpText subItem.score)}}--show-help{{/if}}">

                  <p class="subitem__desc">
-                    {{#unless ../../scored }}
+                    <!--{{#unless ../../scored }}


leaving category in is fine, I'm sure #1463 will have something to say about it anyways, but I'd say just delete this. We definitely don't want.

kaycebasques · 2017-02-01T20:23:01Z

Well, while we're on the topic, since you removed "site", why not also remove "page"? LH always and only audits the current page, no? So using "page" implies some distinction that does not exist.

paulirish

we're using "Avoids" instead of "Page avoids", so i'm thinking we should drop "Page" as well and go verb-first.

"Has a <viewport> with ..."
"Opens external links with rel..."
"Uses HTTP/2 for its own resources"
"Has optimized images"
"Uses passive listeners..."

wdyt?

paulirish · 2017-02-01T20:17:54Z

lighthouse-core/audits/dobetterweb/link-blocking-first-paint.js

@@ -34,7 +34,7 @@ class LinkBlockingFirstPaintAudit extends Audit {
    return {
      category: 'Performance',
      name: 'link-blocking-first-paint',
-      description: 'Site does not use <link> that delay first paint',
+      description: 'Avoids `<link>` that delay first paint',


does this description also go through marked now?

you got it.

kaycebasques · 2017-02-01T20:29:20Z

Dropping page also cleans up a lot of unnecessary text, so that should be right up errone's alleys

kaycebasques · 2017-02-01T20:25:54Z

lighthouse-core/audits/deprecations.js

@@ -33,7 +33,7 @@ class Deprecations extends Audit {
    return {
      category: 'Deprecations',
      name: 'deprecations',
-      description: 'Site does not use deprecated APIs',
+      description: 'Avoids deprecated APIs',


I'm all for nixing Site, but Avoids strikes me as vaguer than Does not use. I suggest s/Avoid/Does not use/ throughout.

The complaint we've been getting (and which this PR tries to address) is that "not" in "X" Does not use.... is confusing. The "X" icon + "not" appear to folks as a double negative.

I also think there could be something better than "avoids" . Any suggestions?

Ah, OK. Hm... Well, we could drop the verb... E.g. Deprecated APIs. Else it might require inversing the titles... E.g. Uses Up-To-Date APIs

Yeah... given the constraints of the current UI, I don't see a better alternative to avoid! Although something doesn't sit well with me about it.

Rides off grumbling into the sunset

ebidel · 2017-02-01T20:37:24Z

K. Dropped "page" from the audit's that use positive assertions in their descriptions

patrickhulce · 2017-02-01T23:35:38Z

lighthouse-core/audits/dobetterweb/script-blocking-first-paint.js

@@ -33,7 +33,7 @@ class ScriptBlockingFirstPaint extends Audit {
    return {
      category: 'Performance',
      name: 'script-blocking-first-paint',
-      description: 'Site does not use <script> in head that delays first paint',
+      description: 'Avoids `<script>` in head that delay first paint',


Why delay? <script> is singular or referential to the idea of script, no?

"Avoids script in head that delays first paint" or
"Avoids scripts in head that delay first paint"

I read "Avoids <script> in head... as "Avoids scripts in head....", but I'll change it back to "Avoids <script> in head that delays first paint"

brendankenny · 2017-02-02T00:05:32Z

maybe clean up the accessibility audits debugString generator? Could just get rid of the rule.help and the parentheses. Example:

kaycebasques · 2017-02-02T00:06:58Z

The learn more after 1 element fails this test can also be removed now that the a11y audits have links to docs.

brendankenny · 2017-02-02T00:08:03Z

Could just get rid of the rule.help and the parentheses

actually maybe the whole debugString since the rule help is contained in the audit description and the "Failed on 1 element` is contained in the extended info header below :)

googlebot · 2017-02-02T01:33:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

paulirish

these edits are so good. thanks!

ebidel · 2017-02-02T18:30:28Z

I'll do the a11y cleanup in another pr

ebidel added 4 commits February 1, 2017 09:47

Update audit description. Remove duplicate debugString for failing au…

e185a9e

…dits

more violation message consolidation

fc7bda8

Leave category out of report

0d72518

consistent wording for positive usage and avoid usage

8e63377

ebidel requested a review from paulirish February 1, 2017 19:17

ebidel commented Feb 1, 2017

View reviewed changes

ebidel added the report label Feb 1, 2017

ebidel added 2 commits February 1, 2017 11:49

fix linting

e291d55

fix tests

2fda477

brendankenny requested changes Feb 1, 2017

View reviewed changes

feedback

c26204a

paulirish reviewed Feb 1, 2017

View reviewed changes

kaycebasques suggested changes Feb 1, 2017

View reviewed changes

Remove "page"

85c6786

kaycebasques mentioned this pull request Feb 1, 2017

update LH titles google/WebFundamentals#4122

Closed

kaycebasques approved these changes Feb 1, 2017

View reviewed changes

patrickhulce reviewed Feb 1, 2017

View reviewed changes

Update uses-responsive-images.js

fa85761

paulirish approved these changes Feb 2, 2017

View reviewed changes

paulirish merged commit a10f6e0 into master Feb 2, 2017

paulirish deleted the lingo branch February 2, 2017 18:26

This was referenced Feb 2, 2017

Remove debugString from accessibility audits + tests. #1606

Merged

Invert the report form for the discouraged API use #1613

Closed

brendankenny mentioned this pull request Feb 3, 2017

Update Manifest gatherer to use gather error instead of -1 artifact #1624

Merged

Report: lingo cleanup, visual cleanup, remove redundant messaging #1598

Report: lingo cleanup, visual cleanup, remove redundant messaging #1598

Conversation

ebidel commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 1, 2017

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

kaycebasques commented Feb 1, 2017

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaycebasques commented Feb 1, 2017

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

ebidel commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Feb 2, 2017

kaycebasques commented Feb 2, 2017

brendankenny commented Feb 2, 2017

googlebot commented Feb 2, 2017

paulirish left a comment

Choose a reason for hiding this comment

ebidel commented Feb 2, 2017