-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding audit for theme-color in HTML #79
Conversation
This generally looks good. I think we should try and resolve #77 before we do any more HTML-based tests, but assuming you're happy to land that (and update this audit) we should be gtg! |
|
||
it('fails when multiple theme-colors exist', () => { | ||
return assert.equal(Audit.audit({ | ||
html: `<!doctype html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we DRY up these html mocks with a single function like:
getMockHtml: function (insert) {
return `...
...
${insert}
....
....`
}
nice work here @gauntface 1 comment and then I am 👍 to land |
828c3c7
to
b5aeded
Compare
@@ -28,6 +28,7 @@ | |||
"chrome-devtools-frontend": "1.0.381789", | |||
"chrome-remote-interface": "^0.11.0", | |||
"devtools-timeline-model": "1.0.16", | |||
"jsdom": "^8.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/samccone/engine-deps
If we want to support older versions of node....
(but I guess since we are using all kinds of es2015 this is a bit of a moot point)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I couldn't even run lighthouse until I upgraded to 5.9 soooooooo
7152df0
to
25ec1a3
Compare
inputs.window.document.querySelectorAll('head meta[name="viewport"]'); | ||
|
||
if (viewportElements.length === 1 && | ||
viewportElements[0].content.indexOf('width=') !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use getAttribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: Why getAttribute() over .content? I have no preference.
f0b1e1c
to
647be36
Compare
We don't want to use jsdom as a dependency here. We will always have the browser available and its DOM is the source of truth. We have to be very conscious about minimizing the dependencies that we need at runtime. Pulling in jsdom and browserifying that into a chrome extension does not work for me. |
jsdom is ignored in the extension since it's not required, that's something. What would you prefer to solve: #77 |
a3aa7b1
to
0c66fe3
Compare
f1f7136
to
a04ab43
Compare
Looks like we need a rebase |
I read in the opensource handbook that you should upset everyone with a big rejection of a new contributor's first PR. Or not. 😕 Again, sorry for being terse and stuff. I feed this thread more flowers for good luck: 🌷 💐 🌹 Discussion on the right approach will continue on #77. |
going to close this for now, as it seems like there is a bit more exploration to do here. @gauntface thanks so much for the PR, and this has totally helped us inform the direction we need to go when gathering and dealing with HTML! I look forward to working more with you on this project! |
@gauntface I'm building off your PR over here: master...paulirish:themecolor Still a WIP I'll have something for you to review soon. |
Might solve #25
cc @paulirish @paullewis