Report: Fix HTML validation errors. #1575
Conversation
| @@ -56,7 +56,7 @@ class ReportGenerator { | |||
|
|
|||
| // Converts a name to a link. | |||
| Handlebars.registerHelper('nameToLink', name => { | |||
| return name.toLowerCase().replace(/\s/, '-'); | |||
| return name.toLowerCase().replace(/\s/g, '-'); | |||
There was a problem hiding this comment.
This is self-explanatory for me but just in case it isn't, say the name is Progressive Web app. Previously it became progressive-web app which is invalid.
There was a problem hiding this comment.
derp. mind attending a test for this?
There was a problem hiding this comment.
Not sure where and how; I haven't looked into tests yet.
There was a problem hiding this comment.
Would this be enough in /lighthouse-core/test/report/report-test.js
it('`nameToLink` works properly', () => {
const reportGenerator = new ReportGenerator();
const html = reportGenerator.generateHTML(sampleResults);
assert.ok(/href="#progressive-web-app"/gm.test(html));
});There was a problem hiding this comment.
Yea, that's probably best for now. We really don't have a great way to test the registered handlebar helpers atm. The few tests we have, look at the generated output :\
Maybe make the asset more specific so there is a smaller chance of false positives in the future. We want make sure "progressive-web-app" is from the nameToLink output and not something else. There's also another place to check:
assert.ok(/class="menu__link" href="#progressive-web-app"/gm.test(html));
assert.ok(/id="progressive-web-app"/gm.test(html));
There was a problem hiding this comment.
could also make the helpers static (and private) functions so they're testable on their own. It would make the tests a little easier to write and run a little faster.
|
The only 2 things left are:
|
| @@ -2,6 +2,7 @@ | |||
| .table_list { | |||
| margin-top: 8px; | |||
| border: 1px solid #EBEBEB; | |||
| border-spacing: 0; | |||
| @@ -56,7 +56,7 @@ class ReportGenerator { | |||
|
|
|||
| // Converts a name to a link. | |||
| Handlebars.registerHelper('nameToLink', name => { | |||
| return name.toLowerCase().replace(/\s/, '-'); | |||
| return name.toLowerCase().replace(/\s/g, '-'); | |||
There was a problem hiding this comment.
derp. mind attending a test for this?
|
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 |
|
Rebased for ya :) |
|
CLAs look good, thanks! |
|
Rebased, should be OK now. Only the |
|
I wouldn't have caught the 2 issues, one with the 404 link and the second with the spaces in the anchors if it weren't for the validator. IMO it's a good safety measure to have it run and be HTML valid as close as possible :) |
BTW, this fixes a 404 link along the way (the link with the double quotes).