From d22d73f5a25e2c4f8683e09dda81d2c2e8aeefe3 Mon Sep 17 00:00:00 2001 From: Adam Raine <6752989+adamraine@users.noreply.github.com> Date: Tue, 26 Oct 2021 13:41:32 -0700 Subject: [PATCH] report(flow): category tooltip highest impact (#13230) --- flow-report/assets/styles.css | 58 ++++++++- flow-report/src/i18n/ui-strings.js | 2 + flow-report/src/summary/category.tsx | 77 +++++++++++- flow-report/src/summary/summary.tsx | 1 + flow-report/src/wrappers/markdown.tsx | 27 ++++ flow-report/test/summary/category-test.tsx | 130 +++++++++++++++++--- flow-report/test/wrappers/markdown-test.tsx | 29 +++++ shared/localization/locales/en-US.json | 3 + shared/localization/locales/en-XL.json | 3 + 9 files changed, 306 insertions(+), 24 deletions(-) create mode 100644 flow-report/src/wrappers/markdown.tsx create mode 100644 flow-report/test/wrappers/markdown-test.tsx diff --git a/flow-report/assets/styles.css b/flow-report/assets/styles.css index 988ae60f78..e0cf824d4e 100644 --- a/flow-report/assets/styles.css +++ b/flow-report/assets/styles.css @@ -31,6 +31,7 @@ --summary-subtitle-font-size: 12px; --summary-title-font-size: 20px; --summary-tooltip-box-shadow-color: rgba(0, 0, 0, 0.25); + --summary-tooltip-line-height: 16px; --topbar-button-font-size: 14px; --topbar-button-size: 45px; --topbar-title-font-size: 14px; @@ -394,22 +395,33 @@ border-radius: 3px; padding: var(--base-spacing); right: 0; + line-height: var(--summary-tooltip-line-height); box-shadow: 0px 4px 4px var(--summary-tooltip-box-shadow-color); z-index: 1; } .SummaryTooltip__title { font-weight: var(--bold-weight); - margin-bottom: var(--half-base-spacing); +} + +.SummaryTooltip__url { + overflow-x: hidden; + text-overflow: ellipsis; + white-space: nowrap; + margin-top: calc(0.25 * var(--base-spacing)); + margin-bottom: calc(0.75 * var(--base-spacing)); + color: var(--color-gray-700); } .SummaryTooltip__category { font-weight: var(--bold-weight); display: flex; + align-items: center; margin-top: var(--half-base-spacing); } .SummaryTooltip__category-title { + line-height: 24px; flex-grow: 1; } @@ -424,6 +436,50 @@ color: var(--color-fail); } +.SummaryTooltip__fraction { + color: var(--color-gray-700); +} + +.SummaryTooltip__informative { + margin-top: calc(0.25 * var(--base-spacing)); + color: var(--color-gray-700); +} + +.SummaryTooltipAudits__title { + line-height: 24px; + margin-top: calc(0.75 * var(--base-spacing)); + font-weight: var(--bold-weight); +} + +.SummaryTooltipAudit { + display: flex; + margin: calc(0.25 * var(--base-spacing)) 0px; + color: var(--color-gray-700); + --score-icon-size: 10px; +} +.SummaryTooltipAudit code { + color: var(--snippet-color); +} +.SummaryTooltipAudit::before { + content: ''; + height: var(--score-icon-size); + width: var(--score-icon-size); + margin-top: calc((var(--summary-tooltip-line-height) - var(--score-icon-size)) / 2); + margin-right: var(--half-base-spacing); +} +.SummaryTooltipAudit--pass::before { + background-color: var(--color-pass); + border-radius: 50%; +} +.SummaryTooltipAudit--average::before { + background-color: var(--color-average); +} +.SummaryTooltipAudit--fail::before { + border-left: calc(var(--score-icon-size) / 2) solid transparent; + border-right: calc(var(--score-icon-size) / 2) solid transparent; + border-bottom: var(--score-icon-size) solid var(--color-fail); +} + .SummaryNavigationHeader { font-size: var(--summary-navigation-header-font-size); line-height: 16px; diff --git a/flow-report/src/i18n/ui-strings.js b/flow-report/src/i18n/ui-strings.js index 68f9e0c261..ab15a272d9 100644 --- a/flow-report/src/i18n/ui-strings.js +++ b/flow-report/src/i18n/ui-strings.js @@ -131,4 +131,6 @@ export const UIStrings = { =1 {{numInformative} informative audit} other {{numInformative} informative audits} }`, + /** Label for a list of Lighthouse audits that are the most impactful. */ + highestImpact: 'Highest impact', }; diff --git a/flow-report/src/summary/category.tsx b/flow-report/src/summary/category.tsx index 6dfbb2593d..99ed8681a1 100644 --- a/flow-report/src/summary/category.tsx +++ b/flow-report/src/summary/category.tsx @@ -10,9 +10,14 @@ import {Util} from '../../../report/renderer/util'; import {Separator} from '../common'; import {CategoryScore} from '../wrappers/category-score'; import {useI18n, useStringFormatter, useLocalizedStrings} from '../i18n/i18n'; +import {Markdown} from '../wrappers/markdown'; import type {UIStringsType} from '../i18n/ui-strings'; +const MAX_TOOLTIP_AUDITS = 2; + +type ScoredAuditRef = LH.ReportResult.AuditRef & {result: {score: number}}; + function getGatherModeLabel(gatherMode: LH.Result.GatherMode, strings: UIStringsType) { switch (gatherMode) { case 'navigation': return strings.navigationReport; @@ -30,10 +35,71 @@ function getCategoryRating(rating: string, strings: UIStringsType) { } } +function getScoreToBeGained(audit: ScoredAuditRef): number { + return audit.weight * (1 - audit.result.score); +} + +function getOverallSavings(audit: LH.ReportResult.AuditRef): number { + return ( + audit.result.details && + audit.result.details.type === 'opportunity' && + audit.result.details.overallSavingsMs + ) || 0; +} + +const SummaryTooltipAudit: FunctionComponent<{audit: LH.ReportResult.AuditRef}> = ({audit}) => { + const rating = Util.calculateRating(audit.result.score, audit.result.scoreDisplayMode); + return ( +
+ +
+ ); +}; + +const SummaryTooltipAudits: FunctionComponent<{category: LH.ReportResult.Category}> = +({category}) => { + const strings = useLocalizedStrings(); + + function isRelevantAudit(audit: LH.ReportResult.AuditRef): audit is ScoredAuditRef { + return audit.result.score !== null && + // Metrics should not be displayed in this group. + audit.group !== 'metrics' && + // Audits in performance without a group are hidden. + (audit.group !== undefined || category.id !== 'performance') && + // We don't want unweighted audits except for opportunities with potential savings. + (audit.weight > 0 || getOverallSavings(audit) > 0) && + // Passing audits should never be high impact. + !Util.showAsPassed(audit.result); + } + + const audits = category.auditRefs + .filter(isRelevantAudit) + .sort((a, b) => { + // Remaining score should always be 0 for perf opportunities because weight is 0. + // In that case, we want to sort by `overallSavingsMs`. + const remainingScoreA = getScoreToBeGained(a); + const remainingScoreB = getScoreToBeGained(b); + if (remainingScoreA !== remainingScoreB) return remainingScoreB - remainingScoreA; + return getOverallSavings(b) - getOverallSavings(a); + }) + .splice(0, MAX_TOOLTIP_AUDITS); + if (!audits.length) return null; + + return ( +
+
{strings.highestImpact}
+ { + audits.map(audit => ) + } +
+ ); +}; + export const SummaryTooltip: FunctionComponent<{ category: LH.ReportResult.Category, - gatherMode: LH.Result.GatherMode -}> = ({category, gatherMode}) => { + gatherMode: LH.Result.GatherMode, + url: string, +}> = ({category, gatherMode, url}) => { const strings = useLocalizedStrings(); const str_ = useStringFormatter(); const { @@ -53,6 +119,7 @@ export const SummaryTooltip: FunctionComponent<{ return (
{getGatherModeLabel(gatherMode, strings)}
+
{url}
@@ -81,6 +148,7 @@ export const SummaryTooltip: FunctionComponent<{ {str_(strings.informativeAuditCount, {numInformative})}
} +
); }; @@ -89,7 +157,8 @@ export const SummaryCategory: FunctionComponent<{ category: LH.ReportResult.Category|undefined, href: string, gatherMode: LH.Result.GatherMode, -}> = ({category, href, gatherMode}) => { + finalUrl: string, +}> = ({category, href, gatherMode, finalUrl}) => { return (
{ @@ -100,7 +169,7 @@ export const SummaryCategory: FunctionComponent<{ href={href} gatherMode={gatherMode} /> - +
:
} diff --git a/flow-report/src/summary/summary.tsx b/flow-report/src/summary/summary.tsx index 24b4e9048b..bac2fa9246 100644 --- a/flow-report/src/summary/summary.tsx +++ b/flow-report/src/summary/summary.tsx @@ -76,6 +76,7 @@ export const SummaryFlowStep: FunctionComponent<{ category={reportResult.categories[c]} href={`#index=${hashIndex}&anchor=${c}`} gatherMode={lhr.gatherMode} + finalUrl={lhr.finalUrl} /> )) } diff --git a/flow-report/src/wrappers/markdown.tsx b/flow-report/src/wrappers/markdown.tsx new file mode 100644 index 0000000000..bb04be772b --- /dev/null +++ b/flow-report/src/wrappers/markdown.tsx @@ -0,0 +1,27 @@ +/** + * @license Copyright 2021 The Lighthouse Authors. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +import {FunctionComponent} from 'preact'; +import {useLayoutEffect, useRef} from 'preact/hooks'; + +import {useReportRenderer} from '../wrappers/report-renderer'; + +export const Markdown: FunctionComponent<{text: string}> = ({text}) => { + const {dom} = useReportRenderer(); + const ref = useRef(null); + + useLayoutEffect(() => { + if (ref.current) { + const md = dom.convertMarkdownCodeSnippets(text); + ref.current.appendChild(md); + } + return () => { + if (ref.current) ref.current.innerHTML = ''; + }; + }, [text]); + + return ; +}; diff --git a/flow-report/test/summary/category-test.tsx b/flow-report/test/summary/category-test.tsx index 14f499eaab..9a9fd08b2e 100644 --- a/flow-report/test/summary/category-test.tsx +++ b/flow-report/test/summary/category-test.tsx @@ -11,6 +11,7 @@ import {SummaryTooltip} from '../../src/summary/category'; import {flowResult} from '../sample-flow'; import {I18nProvider} from '../../src/i18n/i18n'; import {FlowResultContext} from '../../src/util'; +import {ReportRendererProvider} from '../../src/wrappers/report-renderer'; let wrapper: FunctionComponent; @@ -18,9 +19,11 @@ beforeEach(() => { // Include sample flowResult for locale in I18nProvider. wrapper = ({children}) => ( - - {children} - + + + {children} + + ); }); @@ -31,14 +34,16 @@ describe('SummaryTooltip', () => { id: 'performance', score: 1, auditRefs: [ - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 0, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, + /* eslint-disable max-len */ + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 1, group: 'diagnostics'}, + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 1, group: 'diagnostics'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 3'}, weight: 1, group: 'diagnostics'}, + /* eslint-enable max-len */ ], }; const root = render( - , + , {wrapper} ); @@ -46,6 +51,7 @@ describe('SummaryTooltip', () => { expect(() => root.getByText(/^[0-9]+$/)).toThrow(); expect(root.getByText('2 audits passed')).toBeTruthy(); expect(root.getByText('3 passable audits')).toBeTruthy(); + expect(root.getByText('https://example.com')); }); it('renders tooltip without rating', async () => { @@ -53,14 +59,16 @@ describe('SummaryTooltip', () => { id: 'performance', score: 1, auditRefs: [ - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 0, group: 'diagnostics'}, - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 0, group: 'diagnostics'}, - {result: {score: 0, scoreDisplayMode: 'binary'}, weight: 0, group: 'diagnostics'}, + /* eslint-disable max-len */ + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 0, group: 'diagnostics'}, + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 0, group: 'diagnostics'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 3'}, weight: 0, group: 'diagnostics'}, + /* eslint-enable max-len */ ], }; const root = render( - , + , {wrapper} ); @@ -68,6 +76,7 @@ describe('SummaryTooltip', () => { expect(() => root.getByText(/^[0-9]+$/)).toThrow(); expect(root.getByText('2 audits passed')).toBeTruthy(); expect(root.getByText('3 passable audits')).toBeTruthy(); + expect(root.getByText('https://example.com')); }); it('renders scored category tooltip with score', async () => { @@ -75,14 +84,16 @@ describe('SummaryTooltip', () => { id: 'performance', score: 1, auditRefs: [ - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 0, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, + /* eslint-disable max-len */ + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 1, group: 'diagnostics'}, + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 1, group: 'diagnostics'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 3'}, weight: 1, group: 'diagnostics'}, + /* eslint-enable max-len */ ], }; const root = render( - , + , {wrapper} ); @@ -90,6 +101,7 @@ describe('SummaryTooltip', () => { expect(root.getByText('100')).toBeTruthy(); expect(root.getByText('2 audits passed')).toBeTruthy(); expect(root.getByText('3 passable audits')).toBeTruthy(); + expect(root.getByText('https://example.com')); }); it('renders informative audit count if any', async () => { @@ -97,14 +109,16 @@ describe('SummaryTooltip', () => { id: 'performance', score: 1, auditRefs: [ - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 1, scoreDisplayMode: 'binary'}, weight: 1, group: 'diagnostics'}, - {result: {score: 0, scoreDisplayMode: 'informative'}, weight: 1, group: 'diagnostics'}, + /* eslint-disable max-len */ + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 1, group: 'diagnostics'}, + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 1, group: 'diagnostics'}, + {result: {score: null, scoreDisplayMode: 'informative', title: 'Audit 3'}, weight: 1, group: 'diagnostics'}, + /* eslint-enable max-len */ ], }; const root = render( - , + , {wrapper} ); @@ -113,5 +127,83 @@ describe('SummaryTooltip', () => { expect(root.getByText('2 audits passed')).toBeTruthy(); expect(root.getByText('2 passable audits')).toBeTruthy(); expect(root.getByText('1 informative audit')).toBeTruthy(); + expect(root.getByText('https://example.com')); + }); + + it('renders highest impact audits', async () => { + const category: any = { + id: 'seo', + score: 1, + auditRefs: [ + /* eslint-disable max-len */ + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 1, group: 'group'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 2, group: 'group'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 3'}, weight: 3, group: 'group'}, + /* eslint-enable max-len */ + ], + }; + + const root = render( + , + {wrapper} + ); + + const audits = root.getAllByText(/^Audit [0-9]$/); + + expect(root.getByText('Highest impact')).toBeTruthy(); + expect(audits.map(a => a.textContent)).toEqual([ + 'Audit 3', + 'Audit 2', + ]); + }); + + it('renders highest impact audits in performance', async () => { + const category: any = { + id: 'performance', + score: 0.75, + auditRefs: [ + /* eslint-disable max-len */ + {result: {score: 0.75, scoreDisplayMode: 'numeric', title: 'Metric 1'}, weight: 1, group: 'metrics'}, + {result: {score: 0, scoreDisplayMode: 'numeric', title: 'Audit 1', details: {type: 'opportunity', overallSavingsMs: 500}}, weight: 0, group: 'opportunities'}, + {result: {score: 0, scoreDisplayMode: 'numeric', title: 'Audit 2', details: {type: 'opportunity', overallSavingsMs: 1000}}, weight: 0, group: 'opportunities'}, + {result: {score: 0, scoreDisplayMode: 'numeric', title: 'Audit 3', details: {type: 'opportunity', overallSavingsMs: 100}}, weight: 0, group: 'opportunities'}, + /* eslint-enable max-len */ + ], + }; + + const root = render( + , + {wrapper} + ); + + const audits = root.getAllByText(/^(Audit|Metric) [0-9]$/); + + expect(root.getByText('Highest impact')).toBeTruthy(); + expect(audits.map(a => a.textContent)).toEqual([ + 'Audit 2', + 'Audit 1', + ]); + }); + + it('hides highest impact if nothing to show', async () => { + const category: any = { + id: 'performance', + score: 1, + auditRefs: [ + /* eslint-disable max-len */ + {result: {score: 1, scoreDisplayMode: 'binary', title: 'Audit 1'}, weight: 1, group: 'diagnostics'}, + {result: {score: 0, scoreDisplayMode: 'binary', title: 'Audit 2'}, weight: 1}, + {result: {score: null, scoreDisplayMode: 'informative', title: 'Audit 3'}, weight: 1, group: 'diagnostics'}, + /* eslint-enable max-len */ + ], + }; + + const root = render( + , + {wrapper} + ); + + expect(() => root.getByText('Highest impact')).toThrow(); + expect(() => root.getByText(/^Audit [0-9]$/)).toThrow(); }); }); diff --git a/flow-report/test/wrappers/markdown-test.tsx b/flow-report/test/wrappers/markdown-test.tsx new file mode 100644 index 0000000000..5deba8378f --- /dev/null +++ b/flow-report/test/wrappers/markdown-test.tsx @@ -0,0 +1,29 @@ +/** + * @license Copyright 2021 The Lighthouse Authors. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ + +import {render} from '@testing-library/preact'; +import {FunctionComponent} from 'preact'; + +import {Markdown} from '../../src/wrappers/markdown'; +import {ReportRendererProvider} from '../../src/wrappers/report-renderer'; + +let wrapper: FunctionComponent; + +beforeEach(() => { + wrapper = ({children}) => ( + + {children} + + ); +}); + +describe('Markdown', () => { + it('renders markdown text', () => { + const root = render(, {wrapper}); + const text = root.getByText(/^Some.*text$/); + expect(text.innerHTML).toEqual('Some fancy text'); + }); +}); diff --git a/shared/localization/locales/en-US.json b/shared/localization/locales/en-US.json index a498eb7efc..26385916ee 100644 --- a/shared/localization/locales/en-US.json +++ b/shared/localization/locales/en-US.json @@ -59,6 +59,9 @@ "flow-report/src/i18n/ui-strings.js | helpUseCaseTimespan2": { "message": "Discover performance opportunities to improve the experience for long-lived pages and single-page applications." }, + "flow-report/src/i18n/ui-strings.js | highestImpact": { + "message": "Highest impact" + }, "flow-report/src/i18n/ui-strings.js | informativeAuditCount": { "message": "{numInformative, plural,\n =1 {{numInformative} informative audit}\n other {{numInformative} informative audits}\n }" }, diff --git a/shared/localization/locales/en-XL.json b/shared/localization/locales/en-XL.json index 27b3f2ab9b..2b654b66c4 100644 --- a/shared/localization/locales/en-XL.json +++ b/shared/localization/locales/en-XL.json @@ -59,6 +59,9 @@ "flow-report/src/i18n/ui-strings.js | helpUseCaseTimespan2": { "message": "D̂íŝćôv́êŕ p̂ér̂f́ôŕm̂án̂ćê óp̂ṕôŕt̂ún̂ít̂íêś t̂ó îḿp̂ŕôv́ê t́ĥé êx́p̂ér̂íêńĉé f̂ór̂ ĺôńĝ-ĺîv́êd́ p̂áĝéŝ án̂d́ ŝín̂ǵl̂é-p̂áĝé âṕp̂ĺîćât́îón̂ś." }, + "flow-report/src/i18n/ui-strings.js | highestImpact": { + "message": "Ĥíĝh́êśt̂ ím̂ṕâćt̂" + }, "flow-report/src/i18n/ui-strings.js | informativeAuditCount": { "message": "{numInformative, plural,\n =1 {{numInformative} îńf̂ór̂ḿât́îv́ê áûd́ît́}\n other {{numInformative} îńf̂ór̂ḿât́îv́ê áûd́ît́ŝ}\n }" },