Skip to content
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

core(config): show SEO audits in the UI #4057

Merged
merged 6 commits into from
Dec 14, 2017

Conversation

kdzwinel
Copy link
Collaborator

@kdzwinel kdzwinel commented Dec 14, 2017

Latest copy taken from the "Lighthouse SEO Audit Text" doc.

Tested in the extension:


screen shot 2017-12-14 at 19 31 22

screen shot 2017-12-14 at 19 25 57


I made sure that all "learn more" links work and link to the right docs (that's how I found that one of the links was broken ).

Closes #3772

@kdzwinel kdzwinel changed the title Show SEO audits in the UI core(config): show SEO audits in the UI Dec 14, 2017
@rviscomi
Copy link
Member

👍

@@ -20,7 +20,7 @@ class HTTPStatusCode extends Audit {
failureDescription: 'Page has unsuccessful HTTP status code',
helpText: 'Pages with unsuccessful HTTP status codes may not be indexed properly. ' +
'[Learn more]' +
'(https://developers.goole.com/web/tools/lighthouse/audits/successful-http-code).',
'(https://developers.google.com/web/tools/lighthouse/audits/successful-http-code).',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
'seo-crawl': {
title: 'Crawling and Indexing',
description: 'To appear in search results, crawlers need access to your app.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm… I wonder why these groups are not shown in the report as they are in e.g. accessibility:

screen shot 2017-12-14 at 19 56 39

screen shot 2017-12-14 at 19 56 31

@patrickhulce
Copy link
Collaborator

a11y category has its own render logic at the moment, we'll need to refactor to make it part of default logic to render groups if they're present :)

_renderAccessibilityCategory(category, groupDefinitions) {
const element = this._dom.createElement('div', 'lh-category');
this._createPermalinkSpan(element, category.id);
element.appendChild(this._renderCategoryScore(category));
const manualAudits = category.audits.filter(audit => audit.result.manual);
const nonManualAudits = category.audits.filter(audit => !manualAudits.includes(audit));
const auditsGroupedByGroup = /** @type {!Object<string,
{passed: !Array<!ReportRenderer.AuditJSON>,
failed: !Array<!ReportRenderer.AuditJSON>}>} */ ({});
nonManualAudits.forEach(audit => {
const groupId = audit.group;
const groups = auditsGroupedByGroup[groupId] || {passed: [], failed: []};
if (audit.score === 100) {
groups.passed.push(audit);
} else {
groups.failed.push(audit);
}
auditsGroupedByGroup[groupId] = groups;
});

@@ -5,7 +5,7 @@
*/
'use strict';

const parseURL = require('url').parse;
const URL = require('../../lib/url-shim');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that require('url').parse won't work in the extension. Thankfully it was easy to replace.

@kdzwinel
Copy link
Collaborator Author

@patrickhulce thanks! I guess we don't want to create another custom renderer ATM? It doesn't make sense to make this refactor here IMO, it deserves its own PR. @rviscomi are we OK with launching w/o the categories?

@rviscomi
Copy link
Member

Yes, categories would be great to have but shouldn't block this PR.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that plan sgtm!

@kdzwinel any more changes you'd like to make? if not, this lgtm for initial release!

@kdzwinel
Copy link
Collaborator Author

Sorry that I kept adding stuff after opening the PR. I should have put [WIP] in the title. Anyway, I have nothing more to add here. Thank you for reviewing it!

@patrickhulce
Copy link
Collaborator

Sorry that I kept adding stuff after opening the PR. I should have put [WIP] in the title. Anyway, I have nothing more to add here. Thank you for reviewing it!

no worries we all find things after the PR goes up :) this should go out with 2.7 today then!

@patrickhulce patrickhulce merged commit 9008f3c into GoogleChrome:master Dec 14, 2017
@kdzwinel
Copy link
Collaborator Author

🎉 Fantastic! Thanks a bunch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants