feat: add groups to config, accessibility renderer #2057

patrickhulce · 2017-04-24T20:17:36Z

addresses topics item in #1937

I'd prefer to leave UI bikeshedding for when we implement the real version that's going in DevTools, but here's a screenshot

ebidel

This is neat.

ebidel · 2017-04-26T15:50:46Z

lighthouse-core/config/config.js

@@ -568,6 +569,11 @@ class Config {
  get categories() {
    return this._categories;
  }
+
+  /** @type {Object<{title: string, description: string}>} */


closure noob here. can this be:{{title: string, description: string}}

it's an object where the values are {title: string, description: string} but IIRC we actually need Object<string,{title: string, description: string}> now, will update

Ah! That makes more sense :)

ebidel · 2017-04-26T16:38:37Z

lighthouse-core/config/default.js

+        {"id": "bypass", "weight": 1, "tags": ["accessibility-describe-contents"]},
+        {"id": "color-contrast", "weight": 1, "tags": ["accessibility-color-contrast"]},
+        {"id": "definition-list", "weight": 1, "tags": ["accessibility-well-structured"]},
+        {"id": "dlitem", "weight": 1, "tags": ["accessibility-well-structured"]},


nit: we could shorten these to a11y-*. Doesn't all this stuff end up in our json dump?

done, yep it does

ebidel · 2017-04-26T16:39:12Z

lighthouse-core/report/v2/renderer/category-renderer.js

@@ -142,7 +142,16 @@ class CategoryRenderer {
   * @param {!ReportRenderer.CategoryJSON} category
   * @return {!Element}
   */
-  render(category) {
+  render(category, tags) {


tags needs a doc string

ebidel · 2017-04-26T16:39:31Z

lighthouse-core/report/v2/renderer/category-renderer.js

+    }
+  }
+
+  _renderDefaultCategory(category) {


ebidel · 2017-04-26T16:39:33Z

lighthouse-core/report/v2/renderer/category-renderer.js

@@ -167,6 +176,69 @@ class CategoryRenderer {
    element.appendChild(passedElem);
    return element;
  }
+
+  _renderAuditGroup(audits, tag) {


ebidel · 2017-04-26T16:47:55Z

lighthouse-core/report/v2/renderer/category-renderer.js

+      const auditGroupHeader = this._dom.createElement('div', 'lh-audit-group__header');
+      auditGroupHeader.textContent = tag.title;
+
+      const auditGroupDescription = this._dom.createElement('div', 'lh-audit-group__description');


you can style the details arrow like the rest by adding a selector like

lighthouse/lighthouse-core/report/v2/report-styles.css

Lines 228 to 234 in 2ae69be

.lh-score__snippet::-moz-list-bullet {

display: none;

}

.lh-score__snippet::-webkit-details-marker {

display: none;

}

and generating markup like

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

Lines 19 to 22 in 2ae69be

<summary class="lh-score__snippet">

<span class="lh-score__title"></span>

<div class="lh-score__arrow" title="See audits"></div>

</summary>

. lh-score__arrow is the gem.

I saw that but wasn't convinced we wanted to reuse those, lh-score gives me the idea that it's a scored thing specific to audit or category when these are just display groupings. If we want to reuse the same CSS, maybe we can extract it to a different distinct reusable component without tying the category/audit dependency?

Yea, we can rename the CSS if you need to.

"distinct reusable component"....interesting idea. Tell me more :)

"distinct reusable component"....interesting idea. Tell me more

hahaha, I've never had anything against organization of code into components :)

ebidel · 2017-04-26T16:49:07Z

lighthouse-core/report/v2/renderer/category-renderer.js

+      return auditGroupElem;
+  }
+
+  _renderAccessibilityCategory(category, tags) {


ebidel · 2017-04-26T16:53:17Z

lighthouse-core/report/v2/report-styles.css

+}
+
+.lh-audit-group__summary::-webkit-details-marker {
+  float: right;


i dont think you need these styles if using lh-score__arrow

patrickhulce · 2017-04-28T21:30:57Z

rebased against the mega-branch lookin' fly now 😎

ebidel · 2017-04-28T22:19:08Z

Should we have the dropdown arrows on the categories? Otherwise, it's not obvious you can expand them.

patrickhulce · 2017-04-28T22:22:24Z

Should we have the dropdown arrows on the categories? Otherwise, it's not obvious you can expand them.

Yeah we should, fixed

ebidel · 2017-04-28T22:30:55Z

Also now noticing the arrow don't turn. They're always in the down state:

You need some of

lighthouse/lighthouse-core/report/v2/report-styles.css

Line 250 in 9b25941

transform: rotateZ(90deg);

action.

brendankenny

how do you feel about tests for this

brendankenny · 2017-04-28T22:31:11Z

lighthouse-core/config/config.js

@@ -568,6 +569,11 @@ class Config {
  get categories() {
    return this._categories;
  }
+
+  /** @type {Object<string, {title: string, description: string}>} */


I don't believe this would be nullable. Is it |undefined?

if we're bein' real it's * for most of these defs since the config can stick whatever it wants here ;) but yeah could be undefined I'll add

brendankenny · 2017-04-28T22:32:35Z

lighthouse-core/config/default.js

-        {"id": "valid-lang", "weight": 1},
-        {"id": "video-caption", "weight": 1},
-        {"id": "video-description", "weight": 1},
+        {"id": "accesskeys", "weight": 1, "tags": ["a11y-correct-attributes"]},


since this is scoped, should we just call this group or something?

since we're only supporting the first entry, should we just drop the array?

since we're requiring a tag for accessibility audits, should there be a check with a clear warning?

I'm against doing a one off config thing per category when we can re-use the same concept, but are you proposing just renaming tags -> groups ?

since we're requiring a tag for accessibility audits, should there be a check with a clear warning?

sure

since we're only supporting the first entry, should we just drop the array?

accessibility only uses one, but forcing one already seems bad when we just planned last week to achieve the PWA goals with multiple tags on a one

brendan is thinking group making it 1:1 instead of 1:many. So each a11y audit only belongs to a single group. Same with perf.

Doing this means we wouldn't have the ability for an audit to live in two "groups", but that seems decent for now. I can't think of where we actually NEED the extensibility of 1audit:many tags.
So perhaps it's better to keep it simple for now.

@patrickhulce is there a 1:many usecase you're keeping in mind?

I had thought we decided to use tags in the certifications-style rendition of PWA checklist, but if that's a totally distinct "tag" so to speak then I'm down with 1:1 group

talked to paul, I was hearing/thinking random things. certifications are to be totally separate from tags (phew!) and group sgtm 👍

ebidel · 2017-04-28T22:35:23Z

Ah interesting. Looks like we need to be more specific with our selectors for the arrows....now that there are sub sub arrows :)

Changing to this works:
.lh-expandable-details[open] > .lh-expandable-details__summary > .lh-toggle-arrow

patrickhulce · 2017-04-28T23:00:35Z

Changing to this works:
.lh-expandable-details[open] > .lh-expandable-details__summary > .lh-toggle-arrow

yup that's what I got now :)

patrickhulce · 2017-04-28T23:05:53Z

how do you feel about tests for this

like we should definitely have some 👍

patrickhulce · 2017-05-01T21:19:35Z

PTAL this is blocking #2111 :)

brendankenny · 2017-05-01T21:56:52Z

accessibility only uses one, but forcing one already seems bad when we just planned last week to achieve the PWA goals with multiple tags on a one

understood, I'm just thinking if we only have like three uses of this hardcoded into each categoryRenderer, having a generic tags section seems like overkill (general solution for a small number of situations) and less descriptive than it could be (if we just called them what they each were). If we start approaching more uses of these than that, then a generic solution does start looking better.

paulirish · 2017-05-02T16:44:36Z

lgtm on groups refactor

patrickhulce · 2017-05-03T00:07:48Z

@brendankenny you appeased? :)

brendankenny

sweet, LGTM

paulirish · 2017-05-03T01:42:27Z

ELL GEE TEE EMM

feat: add topics to config, accessibility renderer

cfbf8c7

ebidel suggested changes Apr 26, 2017

View reviewed changes

patrickhulce added 3 commits April 26, 2017 13:34

add doctype comments

b7b4f59

extract lh-score__arrow

45c7fdf

indentation huh?

6f924cd

ebidel mentioned this pull request Apr 27, 2017

Adopt web components #2077

Closed

patrickhulce added 2 commits April 27, 2017 15:08

Merge branch 'master' into accessibility_tags

04a6455

Merge branch 'master' into accessibility_tags

f5ead8d

ebidel approved these changes Apr 28, 2017

View reviewed changes

doh

88c78ed

brendankenny requested changes Apr 28, 2017

View reviewed changes

patrickhulce added 2 commits April 28, 2017 16:24

feedback and tests

1378aec

closure comma

b214276

tag -> group

d9d4c99

patrickhulce changed the title ~~feat: add topics to config, accessibility renderer~~ feat: add groups to config, accessibility renderer May 1, 2017

patrickhulce added 2 commits May 2, 2017 09:02

Merge branch 'master' into accessibility_tags

e492ebf

more generic timestamp matching

13d1bed

semicolon

668eb15

brendankenny approved these changes May 3, 2017

View reviewed changes

brendankenny merged commit cee0dea into master May 3, 2017

brendankenny deleted the accessibility_tags branch May 3, 2017 01:40

paulirish added a commit that referenced this pull request May 3, 2017

closure fix followup for #2057

8313bba

paulirish pushed a commit to paulirish/lighthouse that referenced this pull request May 5, 2017

feat: add groups to config, accessibility renderer (GoogleChrome#2057)

769f480

paulirish added a commit to paulirish/lighthouse that referenced this pull request May 5, 2017

closure fix followup for GoogleChrome#2057

4d8aa0d

patrickhulce mentioned this pull request May 11, 2017

Category renderer basics for Perf category #2111

Closed

	.lh-score__snippet::-moz-list-bullet {
	display: none;
	}

	.lh-score__snippet::-webkit-details-marker {
	display: none;
	}

	<summary class="lh-score__snippet">
	<span class="lh-score__title"><!-- fill me --></span>
	<div class="lh-score__arrow" title="See audits"></div>
	</summary>

feat: add groups to config, accessibility renderer #2057

feat: add groups to config, accessibility renderer #2057

Conversation

patrickhulce commented Apr 24, 2017

ebidel 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

patrickhulce Apr 26, 2017 • edited Loading

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

ebidel Apr 26, 2017 • edited Loading

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

patrickhulce commented Apr 28, 2017

ebidel commented Apr 28, 2017

patrickhulce commented Apr 28, 2017

ebidel commented Apr 28, 2017 • edited Loading

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

ebidel commented Apr 28, 2017 • edited Loading

patrickhulce commented Apr 28, 2017 • edited Loading

patrickhulce commented Apr 28, 2017

patrickhulce commented May 1, 2017

brendankenny commented May 1, 2017

paulirish commented May 2, 2017

patrickhulce commented May 3, 2017

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented May 3, 2017

patrickhulce Apr 26, 2017 •

edited

Loading

ebidel Apr 26, 2017 •

edited

Loading

ebidel commented Apr 28, 2017 •

edited

Loading

ebidel commented Apr 28, 2017 •

edited

Loading

patrickhulce commented Apr 28, 2017 •

edited

Loading