Audit for theme-color <meta> #87
Conversation
| @@ -14,14 +14,15 @@ | |||
| "number": true, | |||
| "string": true | |||
| }], | |||
| "no-unused-expressions": [2, { | |||
| "no-unused-expressions": [0, { | |||
There was a problem hiding this comment.
I'd generally say this is a bad sign. What was the use case for this and could it be solved with an inline comment rather than a blanket rule change
There was a problem hiding this comment.
Yah we should probs set a temp bypass if needed. Seems too broad to change generally. I think the _ in arrow functions is normally unused, but that should be a special case we accept I think.
There was a problem hiding this comment.
yeah works for me. we had discussed it, but happy to keep this as is, really.
| @@ -0,0 +1,35 @@ | |||
| /** | |||
| * Copyright 2016 Google Inc. All rights reserved. | |||
There was a problem hiding this comment.
@license headers on all of these please :0
|
Good work here @paulirish :) I am excited to get this landed, since it opens all kinds of doors for more DOM work 💃 |
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
Please take a look? I'm halfway through writing a test for driver.querySelector's return values. It can land in here or the next PR. |
|
Test landed. 🍆 |
| @@ -0,0 +1,40 @@ | |||
| /** | |||
| * Copyright 2016 Google Inc. All rights reserved. | |||
|
🔭 👀 |
| @@ -20,6 +20,10 @@ const Gather = require('./gather'); | |||
|
|
|||
| class Viewport extends Gather { | |||
|
|
|||
| /** | |||
| @param {!{driver: !object}} options | |||
There was a problem hiding this comment.
super nit... we need
* at the start of these lines
|
(let's be sure to squash this into 1 commit before merge ✨ ) |
|
CLAs look good, thanks! |
1 similar comment
|
CLAs look good, thanks! |
|
so squash. |
|
👍 |
| }); | ||
| return driver.querySelector('head link[rel="manifest"]') | ||
| .then(node => node.getAttribute('href')) | ||
| .then(manifestURL => Manifest._loadFromURL(options, manifestURL)) |
There was a problem hiding this comment.
we should document (or file an issue if we want to change it) that since the page isn't fetching the manifest, this might not be the same manifest the page would get (e.g. if the user needs to be logged in)
There was a problem hiding this comment.
although that might just be #83
|
LGTM, only style/annotation issues remaining |
|
@gauntface I tried for like half an hour to keep your PR as a solo commit and squashing my stuff on top, but it was a lot of git-fighting so I gave up. :/ Definitely want your input on this stuff going forward. |
started with @gauntface's #79 PR as a base, and reworked with the DOM approach.
matt, would love your review on this!