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

report(category): enable all categories to show audit groups #4278

Merged
merged 12 commits into from
Feb 21, 2018

Conversation

kdzwinel
Copy link
Collaborator

@kdzwinel kdzwinel commented Jan 17, 2018

Enables audit grouping, previously restricted to accessibility category only, for all categories. Additionally, enables 'non applicable' group for all categories.

Before

screen shot 2018-02-07 at 11 29 08

After

screen shot 2018-02-07 at 11 29 21

Example report: http://lh-audit-grouped.bitballoon.com/

Fixes #4093


/* globals self, Util */

class PerformanceCategoryRenderer extends CategoryRenderer {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@paulirish category-renderer.js was huuuuge. I started refactor by splitting it up into three files (perf, a11y, default). Hope that's OK? In the next step I'll move audit group support from a11y to the default renderer and (hopefully?) nuke the a11y render.

Copy link
Member

Choose a reason for hiding this comment

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

sounds great

@kdzwinel kdzwinel changed the title report(category): enable all categories to show audit groups (WIP) report(category): enable all categories to show audit groups Feb 7, 2018
@@ -0,0 +1,194 @@
/**
Copy link
Collaborator Author

@kdzwinel kdzwinel Feb 7, 2018

Choose a reason for hiding this comment

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

since this is a new file we need to update https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/audits2/module.json#L20 . Because it's missing yarn compile-devtools is currently failing.

[Edit] somehow it passes on travis, no idea what's going on


this._detailsRenderer.setTemplateContext(this._templateContext);
/** @protected {!DOM} */
this.dom = dom;
Copy link
Collaborator Author

@kdzwinel kdzwinel Feb 7, 2018

Choose a reason for hiding this comment

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

yarn compile-devtools forced me to remove _ prefix from protected variables 😿

@kdzwinel
Copy link
Collaborator Author

@patrickhulce @paulirish as far as I remember, we agreed it to leave this as is and don't migrate to getters (this.dom -> this._dom + this.getDOM())?

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

Took this for a spin and things look identical between master and this (except for the reportGroups in SEO)

nice work!

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.

None yet

3 participants