new_audit(dobetterweb): add new audit for doctype #5274

schalkneethling · 2018-05-19T11:39:41Z

[WIP] Adds audit for Doctype

schalkneethling · 2018-05-20T14:57:40Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+  afterPass(passContext) {
+    const driver = passContext.driver;
+
+    return driver.getDocument().then(node => node);


I am hoping this will return document as it does not look like I can call driver.document as what I am really after is document and then from that document.name

rather than using the protocol's DOM methods, let's just grab the doctype via javascript.

define a function declaration in the gatherer.. something like

function getDoctype() { const {name, publicId, systemId} = document.doctype; return {name, publicId, systemId}; }

( i think we should grab the name, publicId, and systemId )

then, on L18 you can do

driver.evaluateAsync(`(${getDoctype.toString()}())`)

see the cache-contents gatherer for a model.

paulirish

Nice start!

You'll want to add this audit to default-config.js so you can actually try it out. ;)

paulirish · 2018-05-21T17:28:48Z

lighthouse-core/audits/dobetterweb/doctype.js

+      };
+    }
+
+    return {


over here we'll assert that .name === 'html'

link up https://html.spec.whatwg.org/multipage/parsing.html#the-initial-insertion-mode in a comment

and i guess that publicId and systemId are blank (if i'm reading the spec right)

paulirish · 2018-05-21T17:44:04Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+  afterPass(passContext) {
+    const driver = passContext.driver;
+
+    return driver.getDocument().then(node => node);


rather than using the protocol's DOM methods, let's just grab the doctype via javascript.

define a function declaration in the gatherer.. something like

function getDoctype() { const {name, publicId, systemId} = document.doctype; return {name, publicId, systemId}; }

( i think we should grab the name, publicId, and systemId )

then, on L18 you can do

driver.evaluateAsync(`(${getDoctype.toString()}())`)

see the cache-contents gatherer for a model.

paulirish · 2018-05-21T17:45:36Z

lighthouse-core/audits/dobetterweb/doctype.js

+    return {
+      name: 'doctype',
+      description: 'Document uses HTML5 doctype',
+      failureDescription: 'Document does not use HTML5 doctype or no doctype found',


Page is missing the HTML doctype

paulirish · 2018-05-21T17:45:51Z

lighthouse-core/audits/dobetterweb/doctype.js

+  static get meta() {
+    return {
+      name: 'doctype',
+      description: 'Document uses HTML5 doctype',


Page has the HTML doctype

schalkneethling · 2018-05-21T17:49:57Z

Great stuff, thanks for the review and feedback @paulirish - I will work on this next round and update the PR.

paulirish · 2018-05-21T18:01:30Z

Cool! btw here's how to add to the config (and temporarily only run this one audit, skipping the rest)

diff --git a/lighthouse-core/config/constants.js b/lighthouse-core/config/constants.js
index a8995d63..e9808423 100644
--- a/lighthouse-core/config/constants.js
+++ b/lighthouse-core/config/constants.js
@@ -42,7 +42,9 @@ const defaultSettings = {
   blockedUrlPatterns: null,
   additionalTraceCategories: null,
   extraHeaders: null,
-  onlyAudits: null,
+  onlyAudits: [
+    'doctype',
+  ],
   onlyCategories: null,
   skipAudits: null,
 };
diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js
index f06ab1f5..cbb0b6b1 100644
--- a/lighthouse-core/config/default-config.js
+++ b/lighthouse-core/config/default-config.js
@@ -32,6 +32,7 @@ module.exports = {
       'dobetterweb/all-event-listeners',
       'dobetterweb/anchors-with-no-rel-noopener',
       'dobetterweb/appcache',
+      'dobetterweb/doctype',
       'dobetterweb/domstats',
       'dobetterweb/js-libraries',
       'dobetterweb/optimized-images',
@@ -162,6 +163,7 @@ module.exports = {
     'byte-efficiency/uses-responsive-images',
     'byte-efficiency/efficient-animated-content',
     'dobetterweb/appcache-manifest',
+    'dobetterweb/doctype',
     'dobetterweb/dom-size',
     'dobetterweb/external-anchors-use-rel-noopener',
     'dobetterweb/geolocation-on-start',
@@ -371,6 +373,7 @@ module.exports = {
         {id: 'no-document-write', weight: 1},
         {id: 'external-anchors-use-rel-noopener', weight: 1},
         {id: 'geolocation-on-start', weight: 1},
+        {id: 'doctype', weight: 1},
         {id: 'no-vulnerable-libraries', weight: 1},
         {id: 'notification-on-start', weight: 1},
         {id: 'deprecations', weight: 1},

and to then iterate quickly you can do

# just run gatherers
lighthouse http://example.com -G

# just run audits from previously gathered artifacts
lighthouse http://example.com -A

# regular complete run but save artifacts to disk
lighthouse http://example.com -GA

brendankenny

for the type checking bug, you'll need to add your new Doctype artifact to the artifacts type definition so that other files know that it's provided: artifacts.d.ts

schalkneethling · 2018-05-21T18:23:28Z

Thanks @brendankenny

schalkneethling · 2018-05-27T17:39:54Z

Sorry folks, have been down with a side of the swine flu. Update coming soon.

schalkneethling · 2018-06-17T11:02:49Z

Alrighty, finally got around to this again. Thanks so much for all of the feedback and guidance. It seems to me to be working so yay! \o/

With that said, running yarn test throws up a bunch of errors which I am finding a hard time correlating to the work I have done here. Let me know once you have had a chance to look over the changes. Thanks again.

evenstensberg

Left some comments / opinions. Nice work!

evenstensberg · 2018-06-17T13:10:40Z

lighthouse-core/audits/dobetterweb/doctype.js

+   * @return {LH.Audit.Product}
+   */
+  static audit(artifacts) {
+    if (artifacts.Doctype === null) {


Just a nitpick here, you can do if(!artifacts.Doctype), and maybe the naming should be lower case?

and maybe the naming should be lower case?

It's uppercase for historical reasons (it automatically uses the the gatherer class name), and we have wanted to change that at some point, but not starting with this PR :)

@ev1stensberg I totally could but, I tend to want to be explicit. And as the compiler/minifier will optimise the code in the end, it seems ok. Thoughts?

I decided to add your suggestion in the end ;)

evenstensberg · 2018-06-17T13:10:57Z

lighthouse-core/audits/dobetterweb/doctype.js

+    /* Note that the value for name is case sensitive,
+       and must be the string `html`. For details see:
+       https://html.spec.whatwg.org/multipage/parsing.html#the-initial-insertion-mode */
+    if (artifacts.Doctype.name.trim() === 'html') {


cleaner to store this in a variable?

evenstensberg · 2018-06-17T13:11:28Z

lighthouse-core/audits/dobetterweb/doctype.js

+      };
+    }
+
+    /* Note that the value for name is case sensitive,


Just mention it's case sensitive and a link like: case sensitive: https://foo.bar

evenstensberg · 2018-06-17T13:11:48Z

lighthouse-core/audits/dobetterweb/doctype.js

+    } else {
+      return {
+        rawValue: false,
+        explanation: 'Doctype name must be the lowercase string html',


discard the?

evenstensberg · 2018-06-17T13:12:40Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+  afterPass(passContext) {
+    const driver = passContext.driver;
+    /** @type {Object<string>} */
+    return driver.evaluateAsync(`(${getDoctype.toString()}())`);


Maybe extract this to a var? A lot of encapsulation

evenstensberg · 2018-06-17T13:12:58Z

lighthouse-core/test/audits/dobetterweb/doctype-test.js

+describe('DOBETTERWEB: doctype audit', () => {
+  it('fails when HTML does not contain a doctype', () => {
+    const auditResult = Audit.audit({
+      Doctype: null,


See lowercase suggestion

evenstensberg · 2018-06-17T13:13:19Z

package.json

@@ -23,9 +23,7 @@
    "build-viewer": "cd ./lighthouse-viewer && yarn build",
    "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.screenshots.json *.devtoolslog.json *.trace.json || true",
    "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
-
    "smoke": "node lighthouse-cli/test/smokehouse/run-smoke.js",


Don't know if these whitespace trims are needed

yeah, these are (annoyingly) trimmed automatically when running one of several different yarn/npm commands, but since no functional changes to the file can just remove the whole file from the PR

This was required because there was a conflict reported in package.json by Github.

brendankenny · 2018-06-17T17:01:52Z

It's kind of buried in the end of the yarn diff:sample-json test output, but you need to run yarn update:sample-json to update the "golden lhr" expectations file since there will be a new audit output in there.

The Viewer test uses that file for expected output as well, so running that command will fix that test too.

brendankenny

thanks for staying with this @schalkneethling!

There's so many interlocking pieces to update now, I think we need to update the contributing guide and hopefully get you to review it after this :)

brendankenny · 2018-06-17T17:28:12Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+/**
+ * Get and return `name`, `publicId`, `systemId` from
+ * `document.doctype`
+ * @return {Object<string>}


I believe for the typescript compiler, Object needs the key and value types, so it's Object<string, string>, though it's only complaining for one of these, so I'm not positive.

Regardless, it would probably be better to do the more specific @return {{name: string, publicId: string, systemId: string}} anyways.

brendankenny · 2018-06-17T17:28:26Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+   */
+  afterPass(passContext) {
+    const driver = passContext.driver;
+    /** @type {Object<string>} */


@type {{name: string, publicId: string, systemId: string}}

brendankenny · 2018-06-17T17:29:25Z

package-lock.json

@@ -0,0 +1,11762 @@
+{


remove file from PR?

brendankenny · 2018-06-17T17:30:24Z

package.json

@@ -23,9 +23,7 @@
    "build-viewer": "cd ./lighthouse-viewer && yarn build",
    "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.screenshots.json *.devtoolslog.json *.trace.json || true",
    "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
-
    "smoke": "node lighthouse-cli/test/smokehouse/run-smoke.js",


yeah, these are (annoyingly) trimmed automatically when running one of several different yarn/npm commands, but since no functional changes to the file can just remove the whole file from the PR

brendankenny · 2018-06-17T17:34:15Z

lighthouse-core/audits/dobetterweb/doctype.js

+        explanation: 'Doctype name must be the lowercase string html',
+      };
+    }
+  }


I think @paulirish also wanted to give audit feedback on the "publicId and systemId should be empty string cases" to get full coverage of the spec? Though is that only xhtml or how does that even happen :)

@paulirish You want me to add checks for those additional properties in this audit? Thanks!

yup!

this applies to HTML pages as well. just make assert those params are empty string.

brendankenny · 2018-06-17T17:35:37Z

lighthouse-core/test/audits/dobetterweb/doctype-test.js

+      Doctype: null,
+    });
+    assert.equal(auditResult.rawValue, false);
+  });


also want to test the empty string, non-"html", and "html" success cases (as well as publicId and systemId if added?)

brendankenny · 2018-06-17T17:44:22Z

typings/artifacts.d.ts

@@ -53,6 +53,8 @@ declare global {
      CrawlableLinks: {href: string, text: string}[];
      /** CSS coverage information for styles used by page's final state. */
      CSSUsage: {rules: Crdp.CSS.RuleUsage[], stylesheets: Artifacts.CSSStyleSheetInfo[]};
+      /** Information on the document's Doctype, specifically the name, publicId, and systemId */


maybe also "Properties will be the empty string if not present on the doctype." or something in the comment? I was expecting undefined or whatever but at least publicId and systemId are specced to be the empty string if not present.

Also probably something like /** Information on the document's doctype (or null if not present), specifically...

brendankenny · 2018-06-17T17:44:49Z

typings/artifacts.d.ts

@@ -53,6 +53,8 @@ declare global {
      CrawlableLinks: {href: string, text: string}[];
      /** CSS coverage information for styles used by page's final state. */
      CSSUsage: {rules: Crdp.CSS.RuleUsage[], stylesheets: Artifacts.CSSStyleSheetInfo[]};
+      /** Information on the document's Doctype, specifically the name, publicId, and systemId */
+      Doctype: Artifacts.Doctype;


Doctype: Artifacts.Doctype | null;

brendankenny · 2018-06-17T17:48:04Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+ * @return {Object<string>}
+ */
+function getDoctype() {
+  const {name, publicId, systemId} = document.doctype; // eslint-disable-line no-undef


this will throw if there's no doctype, so I guess have an early return as well?

if (!document.doctype) { return null; } ...

much better, thx

schalkneethling · 2018-06-17T18:16:02Z

Thank you for the review everyone. Updated.

schalkneethling · 2018-06-17T18:18:23Z

Oh I see one failing test when running yarn test but seems unrelated:

schalkneethling · 2018-06-20T13:39:19Z

@paulirish @brendankenny Ok, I added tests for systemId and publicId.

I am seeing a new TS error now:

lighthouse-core/gather/gatherers/dobetterweb/doctype.js(17,5): error TS2322: Type 'undefined' is not assignable to type '{ name: string; publicId: string; systemId: string; }'.

I am guessing it is happening because the function could return undefined here:
https://github.com/GoogleChrome/lighthouse/pull/5274/files#diff-5bd8ddd9e30281c7e7ec5103a145d002R17

paulirish · 2018-06-20T14:29:00Z

 /**
  * Get and return `name`, `publicId`, `systemId` from
  * `document.doctype`
- * @return {{name: string, publicId: string, systemId: string}}
+ * @return {{name: string, publicId: string, systemId: string}=}
  */
 function getDoctype() {
   if (!document.doctype) { // eslint-disable-line no-undef

schalkneethling · 2018-06-20T15:35:22Z

Thanks @paulirish updated.

schalkneethling · 2018-06-20T15:37:48Z

Also resolved the conflict, let me know and I should rebase if required.

schalkneethling · 2018-06-25T14:41:28Z

@paulirish @brendankenny Are there still some items outstanding in the PR? Thanks!

brendankenny · 2018-06-25T16:34:35Z

Not sure why appveyor keeps failing the smoke tests for this PR but Travis does not. We'll have to investigate.

brendankenny · 2018-06-25T19:58:00Z

Ah, I thought I saw a different failure before, but the failure now is also on master. Could be a Canary (appveyor, local) vs Stable (travis) issue? (we're investigating)

paulirish · 2018-06-26T21:06:21Z

lighthouse-core/audits/dobetterweb/doctype.js

+   */
+  static get meta() {
+    return {
+      name: 'doctype',


paulirish · 2018-06-26T21:06:25Z

lighthouse-core/audits/dobetterweb/doctype.js

+  static get meta() {
+    return {
+      name: 'doctype',
+      description: 'Page has the HTML doctype',


these naming changes are something you'll notice when you rebase to master ;)

paulirish · 2018-06-26T21:06:33Z

lighthouse-core/audits/dobetterweb/doctype.js

+    return {
+      name: 'doctype',
+      description: 'Page has the HTML doctype',
+      failureDescription: 'Page is missing the HTML doctype',


failureTitle

paulirish · 2018-06-26T21:06:37Z

lighthouse-core/audits/dobetterweb/doctype.js

+      name: 'doctype',
+      description: 'Page has the HTML doctype',
+      failureDescription: 'Page is missing the HTML doctype',
+      helpText: 'Specifying a doctype prevents the browser from switching to quirks-mode.' +


description

paulirish · 2018-06-26T21:09:34Z

lighthouse-core/audits/dobetterweb/doctype.js

+      };
+    }
+
+    if (doctypeName.length === 0) {


i think you can nuke this and let the L72 case handle it.

paulirish · 2018-06-26T21:13:01Z

lighthouse-core/audits/dobetterweb/doctype.js

+    } else {
+      return {
+        rawValue: false,
+        explanation: 'Doctype name must be the lowercase string html',


i'd put html in backticks, though.

paulirish · 2018-06-26T21:22:10Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+ */
+function getDoctype() {
+  if (!document.doctype) { // eslint-disable-line no-undef
+    return;


let's return null instead

paulirish · 2018-06-26T21:22:38Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+/**
+ * Get and return `name`, `publicId`, `systemId` from
+ * `document.doctype`
+ * @return {{name: string, publicId: string, systemId: string}=}


{{name: string, publicId: string, systemId: string} | null}

paulirish · 2018-06-26T21:23:02Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+   */
+  afterPass(passContext) {
+    const driver = passContext.driver;
+    /** @type {{name: string, publicId: string, systemId: string}} */


you can delete this, you're covered by @return {Promise<LH.Artifacts['Doctype']>}

paulirish · 2018-06-26T21:24:46Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

+ * @return {{name: string, publicId: string, systemId: string}=}
+ */
+function getDoctype() {
+  if (!document.doctype) { // eslint-disable-line no-undef


btw wanna add a comment saying that https://www.warnerbros.com/archive/spacejam/movie/jam.htm is one such site with a null doctype? :)

schalkneethling · 2018-07-05T11:17:14Z

@paulirish @brendankenny updated. Apologies for how long this has taken.

evenstensberg

Left some comments 👍

evenstensberg · 2018-07-05T14:31:58Z

lighthouse-core/test/audits/dobetterweb/doctype-test.js

@@ -0,0 +1,92 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


evenstensberg · 2018-07-05T14:32:02Z

lighthouse-core/gather/gatherers/dobetterweb/doctype.js

@@ -0,0 +1,36 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


evenstensberg · 2018-07-05T14:32:09Z

lighthouse-core/audits/dobetterweb/doctype.js

@@ -0,0 +1,73 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


evenstensberg · 2018-07-05T14:32:55Z

lighthouse-core/test/results/sample_v2.json

@@ -3287,7 +3300,7 @@
        }
      ],
      "id": "best-practices",
-      "score": 0
+      "score": null


Why is this null?

I believe this file is generated by update:sample-json so, not something I specifically did in this PR. Perhaps I am wrong? @paulirish @brendankenny

That's right @schalkneethling, but it's null because your audit is erroring when run on our fixtures
because you added a new artifact. Not ideal that we're adding an erroring audit to our golden, do you think you could manually add an expected Doctype artifact to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/test/results/artifacts/artifacts.json?

Thank you for the feedback @patrickhulce. I am happy to add it but, I probably need some guidance. Thanks!

paulirish · 2018-07-06T16:34:41Z

Open that artifacts.json up and add a Doctype object in there with 3 properties in it: name systemId publicId. With values we'd consider to be passing.

…

On Fri, Jul 6, 2018, 9:22 AM Schalk Neethling ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lighthouse-core/test/results/sample_v2.json <#5274 (comment)> : > @@ -3287,7 +3300,7 @@ } ], "id": "best-practices", - "score": 0 + "score": null Thank you for the feedback @patrickhulce <https://github.com/patrickhulce>. I am happy to add it but, I probably need some guidance. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5274 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACZF1cGYAUrY2FRGZHNLwnkBS4W9UAtks5uD468gaJpZM4UFrfl> .

schalkneethling · 2018-07-06T16:59:18Z

@paulirish Updates. I believe I did it correctly. The value is no longer null

schalkneethling · 2018-07-07T11:21:43Z

Fixed the failing tests. Let's see if we can get an all green build 😃

vacation!

paulirish · 2018-07-09T19:51:27Z

Looks great!

Nice job @schalkneethling and thanks for working with us on this one. :)

We're happy to have your contribution and happy to have this audit.

schalkneethling · 2018-07-10T08:23:36Z

Thanks for all the support on this one everyone!

schalkneethling requested review from brendankenny, patrickhulce and paulirish as code owners May 19, 2018 11:39

paulirish deployed to PR staging May 19, 2018 11:39 View deployment

schalkneethling mentioned this pull request May 19, 2018

Audit doctype #5213

Closed

schalkneethling commented May 20, 2018

View reviewed changes

paulirish requested changes May 21, 2018

View reviewed changes

brendankenny reviewed May 21, 2018

View reviewed changes

paulirish added the waiting4committer label May 24, 2018

evenstensberg reviewed Jun 17, 2018

View reviewed changes

brendankenny changed the title ~~new_audit(dobetterweb): add new audit for doctype.~~ new_audit(dobetterweb): add new audit for doctype Jun 17, 2018

brendankenny previously requested changes Jun 17, 2018

View reviewed changes

paulirish requested changes Jun 26, 2018

View reviewed changes

new_audit(dobetterweb): add new audit for doctype Closes #5213

e0bc7c8

evenstensberg reviewed Jul 5, 2018

View reviewed changes

Changes based on review feedback

af86295

Add Doctype to artifacts.json

6b7d373

Fix failing tests

1b8eb76

paulirish approved these changes Jul 9, 2018

View reviewed changes

paulirish merged commit 9e0341a into GoogleChrome:master Jul 9, 2018

zcorpan mentioned this pull request Nov 26, 2019

Doctype check misses some quirky doctypes #10030

Closed

		@@ -0,0 +1,92 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

		@@ -0,0 +1,36 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

		@@ -0,0 +1,73 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

new_audit(dobetterweb): add new audit for doctype #5274

new_audit(dobetterweb): add new audit for doctype #5274

Conversation

schalkneethling commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented May 21, 2018

paulirish commented May 21, 2018

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

schalkneethling commented May 21, 2018

schalkneethling commented May 27, 2018

schalkneethling commented Jun 17, 2018

evenstensberg 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

brendankenny Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 17, 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

brendankenny Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 17, 2018 • 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

brendankenny Jun 17, 2018 • 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

schalkneethling commented Jun 17, 2018

schalkneethling commented Jun 17, 2018

schalkneethling commented Jun 20, 2018

paulirish commented Jun 20, 2018

schalkneethling commented Jun 20, 2018

schalkneethling commented Jun 20, 2018

schalkneethling commented Jun 25, 2018

brendankenny commented Jun 25, 2018

brendankenny commented Jun 25, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Jul 5, 2018

evenstensberg 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

brendankenny left a comment •

edited

Loading

brendankenny Jun 17, 2018 •

edited

Loading

brendankenny Jun 17, 2018 •

edited

Loading

brendankenny Jun 17, 2018 •

edited

Loading

brendankenny Jun 17, 2018 •

edited

Loading

brendankenny commented Jun 25, 2018 •

edited

Loading