-
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
report(redesign): add toggle control to show metric descriptions #8661
Changes from all commits
f46a9d2
c3d2435
e2e5525
e9d1b1a
91f7da3
58985f3
36ed153
ce01a5a
6cfe786
60696ec
a52013e
99e8447
50a0a1a
96dd356
41bb62b
7989080
e8f2382
9b52229
a9186e9
72078d5
a8f0ae4
ddcbebf
42b439c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,13 +60,33 @@ | |
<span class="lh-audit-group__title"></span> | ||
<span class="lh-audit-group__itemcount"></span> | ||
<!-- .lh-audit-group__description will be added here --> | ||
<!-- .lh-metrics-toggle will be added here --> | ||
</div> | ||
<div class=""></div> | ||
</div> | ||
</summary> | ||
</details> | ||
</template> | ||
|
||
<!-- Lighthouse metrics toggle --> | ||
<template id="tmpl-lh-metrics-toggle"> | ||
<div class="lh-metrics-toggle"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd like to use a we shouldn't need JS for this guy (except for this guide seems to be solid on implementation.. also including their resources at the bottom. https://webdesign.tutsplus.com/tutorials/how-to-make-custom-accessible-checkboxes-and-radio-buttons--cms-32074 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, hows that? |
||
<input class="lh-metrics-toggle__input" type="checkbox" name="toggle-metric-descriptions"> | ||
<label for="toggle-metric-descriptions"> | ||
<div class="lh-metrics-toggle__icon lh-metrics-toggle__icon--less" aria-hidden="true"> | ||
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="24" height="24" viewBox="0 0 24 24"> | ||
<path class="lh-metrics-toggle__lines" d="M4 9h16v2H4zm0 4h10v2H4z" /> | ||
</svg> | ||
</div> | ||
<div class="lh-metrics-toggle__icon lh-metrics-toggle__icon--more" aria-hidden="true"> | ||
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"> | ||
<path class="lh-metrics-toggle__lines" d="M3 18h12v-2H3v2zM3 6v2h18V6H3zm0 7h18v-2H3v2z" /> | ||
</svg> | ||
</div> | ||
</label> | ||
</div> | ||
</template> | ||
|
||
<!-- Lighthouse audit --> | ||
<template id="tmpl-lh-audit"> | ||
<div class="lh-audit"> | ||
|
@@ -88,10 +108,10 @@ | |
<!-- Lighthouse perf metric --> | ||
<template id="tmpl-lh-metric"> | ||
<div class="lh-metric"> | ||
<div class="lh-metric__innerwrap tooltip-boundary"> | ||
<div class="lh-metric__innerwrap"> | ||
<span class="lh-metric__title"></span> | ||
<div class="lh-metric__value"></div> | ||
<div class="lh-metric__description tooltip"></div> | ||
<div class="lh-metric__description"></div> | ||
</div> | ||
</div> | ||
</template> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,11 +26,23 @@ const TEMPLATE_FILE_REPORT = fs.readFileSync(__dirname + | |
'/../../../../report/html/report-template.html', 'utf8'); | ||
|
||
describe('ReportUIFeatures', () => { | ||
let renderer; | ||
let reportUIFeatures; | ||
let sampleResults; | ||
let dom; | ||
|
||
/** | ||
* @param {LH.JSON} lhr | ||
*/ | ||
function render(lhr) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. render function looks 👍 main issue is that it can be pretty slow, so we want to share rendered reports between tests as much as possible, but that's what you do here, so looks good to me |
||
const detailsRenderer = new DetailsRenderer(dom); | ||
const categoryRenderer = new CategoryRenderer(dom, detailsRenderer); | ||
const renderer = new ReportRenderer(dom, categoryRenderer); | ||
const reportUIFeatures = new ReportUIFeatures(dom); | ||
const container = dom.find('main', dom._document); | ||
renderer.renderReport(lhr, container); | ||
reportUIFeatures.initFeatures(lhr); | ||
return container; | ||
} | ||
|
||
beforeAll(() => { | ||
global.Util = Util; | ||
global.ReportUIFeatures = ReportUIFeatures; | ||
|
@@ -72,11 +84,7 @@ describe('ReportUIFeatures', () => { | |
}; | ||
|
||
dom = new DOM(document.window.document); | ||
const detailsRenderer = new DetailsRenderer(dom); | ||
const categoryRenderer = new CategoryRenderer(dom, detailsRenderer); | ||
renderer = new ReportRenderer(dom, categoryRenderer); | ||
sampleResults = Util.prepareReportResult(sampleResultsOrig); | ||
reportUIFeatures = new ReportUIFeatures(dom); | ||
}); | ||
|
||
afterAll(() => { | ||
|
@@ -96,16 +104,11 @@ describe('ReportUIFeatures', () => { | |
|
||
describe('initFeatures', () => { | ||
it('should init a report', () => { | ||
// render a report onto the UIFeature dom | ||
const container = reportUIFeatures._dom._document.querySelector('main'); | ||
renderer.renderReport(sampleResults, container); | ||
|
||
assert.equal(reportUIFeatures.json, undefined); | ||
reportUIFeatures.initFeatures(sampleResults); | ||
assert.ok(reportUIFeatures.json); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. testing We could definitely do something much better if you want to come up with something to test :) or just restore and we'll think about it another day There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about just removing it? Doesn't seem useful now that we have all these other tests checking individual behaviors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
seems fine now, yeah |
||
const container = render(sampleResults); | ||
assert.equal(dom.findAll('.lh-category', container).length, 5); | ||
}); | ||
|
||
describe('thrid-party filtering', () => { | ||
describe('third-party filtering', () => { | ||
let container; | ||
|
||
beforeAll(() => { | ||
|
@@ -129,9 +132,7 @@ describe('ReportUIFeatures', () => { | |
]; | ||
|
||
// render a report onto the UIFeature dom | ||
container = dom.find('main', dom._document); | ||
renderer.renderReport(lhr, container); | ||
reportUIFeatures.initFeatures(lhr); | ||
container = render(lhr); | ||
}); | ||
|
||
it('filters out third party resources in details tables when checkbox is clicked', () => { | ||
|
@@ -161,4 +162,41 @@ describe('ReportUIFeatures', () => { | |
}); | ||
}); | ||
}); | ||
|
||
describe('metric description toggles', () => { | ||
let container; | ||
let metricsAuditGroup; | ||
let toggle; | ||
const metricsClass = 'lh-audit-group--metrics'; | ||
const toggleClass = 'lh-metrics-toggle__input'; | ||
const showClass = 'lh-audit-group--metrics__show-descriptions'; | ||
|
||
describe('works if there is a performance category', () => { | ||
beforeAll(() => { | ||
container = render(sampleResults); | ||
metricsAuditGroup = dom.find(`.${metricsClass}`, container); | ||
toggle = dom.find(`.${toggleClass}`, metricsAuditGroup); | ||
}); | ||
|
||
it('descriptions hidden by default', () => { | ||
assert.ok(!metricsAuditGroup.classList.contains(showClass)); | ||
}); | ||
|
||
it('can toggle description visibility', () => { | ||
assert.ok(!metricsAuditGroup.classList.contains(showClass)); | ||
toggle.click(); | ||
assert.ok(metricsAuditGroup.classList.contains(showClass)); | ||
toggle.click(); | ||
assert.ok(!metricsAuditGroup.classList.contains(showClass)); | ||
}); | ||
}); | ||
|
||
it('report still works if performance category does not run', () => { | ||
const lhr = JSON.parse(JSON.stringify(sampleResults)); | ||
delete lhr.categories.performance; | ||
container = render(lhr); | ||
assert.ok(!container.querySelector(`.${metricsClass}`)); | ||
assert.ok(!container.querySelector(`.${toggleClass}`)); | ||
}); | ||
}); | ||
}); |
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.
the base
.lh-metrics-toggle__icon
rule could assume the--less
case so this rule isnt necessaryregardless you can move width/height into the base
lh-metrics-toggle__icon
rule