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: expand groups within Passed Audits #6930

Merged
merged 5 commits into from
Jan 11, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions lighthouse-core/report/html/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,9 @@ class CategoryRenderer {
* array of audit and audit-group elements.
* @param {Array<LH.ReportResult.AuditRef>} auditRefs
* @param {Object<string, LH.Result.ReportGroup>} groupDefinitions
* @param {{expandable: boolean}} opts
* @return {Array<Element>}
*/
_renderGroupedAudits(auditRefs, groupDefinitions, opts) {
_renderGroupedAudits(auditRefs, groupDefinitions) {
// Audits grouped by their group (or under notAGroup).
/** @type {Map<string, Array<LH.ReportResult.AuditRef>>} */
const grouped = new Map();
Expand Down Expand Up @@ -252,7 +251,10 @@ class CategoryRenderer {

// Push grouped audits as a group.
const groupDef = groupDefinitions[groupId];
const auditGroupElem = this.renderAuditGroup(groupDef, opts);
// Expanded by default!
// 1. The 'failed' clump has all groups expanded.
// 2. If a clump is collapsed (passed, n/a), we want the groups within to already be expanded
const auditGroupElem = this.renderAuditGroup(groupDef, {expandable: false});
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a little confusing, it's probably because I don't remember what our opts actually do at this point 😆 so expandable means isExpansionToggleable and it's expanded by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

agree it's a funny name. expandable means collapsedAndCanBeExpanded

for (const auditRef of groupAuditRefs) {
auditGroupElem.appendChild(this.renderAudit(auditRef, index++));
}
Expand All @@ -272,7 +274,7 @@ class CategoryRenderer {
*/
renderUnexpandableClump(auditRefs, groupDefinitions) {
Copy link
Member

Choose a reason for hiding this comment

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

weren't you going to rename?

Copy link
Member

Choose a reason for hiding this comment

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

weren't you going to rename?

oh, maybe because pwa-category-renderer.js also uses it?

const clumpElement = this.dom.createElement('div');
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable: false});
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions);
elements.forEach(elem => clumpElement.appendChild(elem));
return clumpElement;
}
Expand All @@ -291,7 +293,8 @@ class CategoryRenderer {
* ├── audit 5
* └── audit 6
* clump (e.g. 'manual')
* ├── …
* ├── audit 1
* ├── audit 2
* ⋮
Copy link
Member

Choose a reason for hiding this comment

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

maybe

/**
 * Renders a clump (a top level report section), under a status of failed, manual,
 * passed, or notApplicable. The result ends up something like:
 *
 * failed clump
 *   ├── audit 1 (w/o group)
 *   ├── audit 2 (w/o group)
 *   ├── audit group
 *   |  ├── audit 3
 *   |  └── audit 4
 *   └── audit group
 *      ├── audit 5
 *      └── audit 6
 * other clump (e.g. 'manual')
 *   ├── audit 1
 *   ├── audit 2
 *   ├── …
 *   ⋮

* @param {TopLevelClumpId} clumpId
* @param {{auditRefs: Array<LH.ReportResult.AuditRef>, groupDefinitions: Object<string, LH.Result.ReportGroup>, description?: string}} clumpOpts
Expand All @@ -305,17 +308,15 @@ class CategoryRenderer {
return failedElem;
}

const expandable = true;
const elements = this._renderGroupedAudits(auditRefs, groupDefinitions, {expandable});

const clumpInfo = this._clumpDisplayInfo[clumpId];
// TODO: renderAuditGroup shouldn't be used to render a clump (since it *contains* audit groups).
Copy link
Member

Choose a reason for hiding this comment

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

this TODO makes no sense now. Maybe // TODO: renderAuditGroup shouldn't be used to render a clump since it's a clump, not a group.

const groupDef = {title: clumpInfo.title, description};
const opts = {expandable, itemCount: auditRefs.length};
const clumpElem = this.renderAuditGroup(groupDef, opts);
const groupOpts = {expandable: true, itemCount: auditRefs.length};
const clumpElem = this.renderAuditGroup(groupDef, groupOpts);
clumpElem.classList.add('lh-clump', clumpInfo.className);

elements.forEach(elem => clumpElem.appendChild(elem));
// For all non-failed clumps, we don't group
Copy link
Member

Choose a reason for hiding this comment

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

This might not make much sense outside the context of this PR. Just // Add all audit results to the clump. (or add an intermediate const auditElems = auditRefs.map(...) and can probably avoid the comment altogether)

clumpElem.append(...auditRefs.map(this.renderAudit.bind(this)));

return clumpElem;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,16 @@ describe('CategoryRenderer', () => {
assert.equal(failedAuditGroups.length, failedAuditTags.size);
});

it('renders the passed audits grouped by group', () => {
it('renders the passed audits ungrouped', () => {
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
const passedAudits = category.auditRefs.filter(audit =>
audit.result.scoreDisplayMode !== 'notApplicable' && audit.result.score === 1);
const passedAuditTags = new Set(passedAudits.map(audit => audit.group));

const passedAuditGroups = categoryDOM.querySelectorAll('.lh-clump--passed .lh-audit-group');
assert.equal(passedAuditGroups.length, passedAuditTags.size);
const passedAuditsElems = categoryDOM.querySelectorAll('.lh-clump--passed .lh-audit');

assert.equal(passedAuditGroups.length, 0);
assert.equal(passedAuditsElems.length, passedAudits.length);
});

it('renders all the audits', () => {
Expand Down Expand Up @@ -293,15 +295,20 @@ describe('CategoryRenderer', () => {
});

it('gives each group a selectable class', () => {
const categoryClone = JSON.parse(JSON.stringify(category));
// Force all results to be Failed for accurate counting of groups.
categoryClone.auditRefs.forEach(ref => {
ref.result.score = 0;
ref.result.scoreDisplayMode = 'binary';
});
const categoryGroupIds = new Set(category.auditRefs.filter(a => a.group).map(a => a.group));
assert.ok(categoryGroupIds.size > 6); // Ensure there's something to test.

const categoryElem = renderer.render(category, sampleResults.categoryGroups);
const categoryElem = renderer.render(categoryClone, sampleResults.categoryGroups);

categoryGroupIds.forEach(groupId => {
const selector = `.lh-audit-group--${groupId}`;
// Could be multiple results (e.g. found in both passed and failed clumps).
assert.ok(categoryElem.querySelectorAll(selector).length >= 1,
assert.equal(categoryElem.querySelectorAll(selector).length, 1,
`could not find '${selector}'`);
});
});
Expand Down