-
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: introduce the new report api, add dom.rootEl #13277
Conversation
…esnt scope well already. about to revert
…d, it doesnt scope well already. about to revert" This reverts commit 7779f25.
*/ | ||
export function renderReport(lhr, opts = {}) { | ||
const rootEl = document.createElement('main'); | ||
rootEl.classList.add('lh-root', 'lh-vars'); |
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 standalone template HTML already includes these classes in a main
tag. WDYT about making rootEl
a normal div, and removing the classes from the template files?
Standalone renderer would still look for the main
tag.
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.
basically did the same kinda thing. just avoiding a double-main scenario. :)
|
||
reportFragment.appendChild(topbarDocumentFragment); | ||
if (!this._opts.omitTopbar) { |
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 topbar is removed separately in report.tsx
. Can we replace that logic with this new option now?
report/renderer/dom.js
Outdated
@@ -216,6 +229,8 @@ export class DOM { | |||
} | |||
|
|||
/** | |||
* ONLY use if `dom.rootEl` isn't sufficient for your needs. It is preferred for all scoping, |
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.
Got a little confused on first read
* ONLY use if `dom.rootEl` isn't sufficient for your needs. It is preferred for all scoping, | |
* ONLY use if `dom.rootEl` isn't sufficient for your needs. `dom.rootEl` is preferred for all scoping, |
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.
thx
viewer/app/index.html
Outdated
|
||
<div class="drop_zone"></div> | ||
<div class="renderer-container"></div> |
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.
FWIW my suggestion was to keep this as a main
tag and have generated report contents as div
s. Generating main
in renderReport
might create nested main
s in the flow report.
<noscript>Lighthouse report requires JavaScript. Please enable.</noscript> | ||
|
||
<main><!-- report populated here --></main> | ||
<main class="flow-vars lh-root lh-vars"><!-- report populated here --></main> |
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.
Todo for me to add these programmatically in the flow report.
@@ -21,12 +21,11 @@ | |||
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1"> | |||
<link rel="icon" href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAEhklEQVR4AWJxL/BhIAesev1U5tcflpncgNrKIsqNIwzC9feMpDUzs70kOczMzMzJJcxwCTMzncPMnOwtzBwzMzPb0vRfeZPp0VhPS5I39V5fdiXV1/VD+9QC7OVn9BsyH1XIoEI1PfmJvLFowVV564+34DFUHudbmfDh4kVXh//7XwE+WjS/YfXZe3yr4j2rqj1AIhSB7hZ8ZtPZu/zw8cK523U4wE1/rvPfWrz4zs0m9ZdC9yUJAlASdBAgocRegfF/f3/h/PuaFsxMdwjAR0vm1+06eMMfIrhLqTWqdH4EumU2SPfMhigJAlRQbZrgrRsl9U+Y2DYDFCz3ILC9kiAiqSrMwbWT0nceEnR+9Kggc2zjOJCASDENkg0a5HfZZgDP81CM3CrQs2Z1+o7DJ6ePr8sK0AOCHv5Jjdt3evyYSaZ351VIStIxPRAUtrBYbxC6w+BZ0ivVSBKkIhJhemSyZpfB00EiPO2VjzYkxhcqXQqCWCShGplvi3y0QxqbuBurMjyJeWnkHZuAEgIQGsUBqwrfjZ+IlBgKyRJzVVYF8O6qFWdh86YzQzMrZigYmxAyfvHgLZQ/LC1CbeniW2Hkqr/PH16SgvGuf2/uzNMBwJA/njxizGPtSyAf7EziJCMGRDRdhoAC4PL1A/SrKQMAAQkEfpJAcRQdrBJ7gNwjSpJsdwK+CANBkqa1LgQB4IicV9nYUct7gaxuDJUErQIiEAiMxLVOFlKzIktPpT0ggpdpC/8YAHnxbgkUY4tAAFSR7AAXNyAAWHJrA/kHGjzg5nleuwFO7Nd/IoDw4Pm58+4jNLmYG0wRA5bErc2Mr3Y+dXTDW1VvwqbJkzMCHQ4S1GTCBOIgUHJrGdEwqzR+jAp/o2qAZelUDoQnruEEdDclJI6576AlNVfc+22XN/+Y1vnJD0Yind6UpEEvn/Hqq15EYjCW7jZCJEpnNvDgkyelDjs106kuux2AAXCSobULOWP8mLhYlpoDMK4qAFXJGk+grtH8YXVz5KJblqaG1+VUdTc0I290bmUQAriGITRbdQnom0aoFj8kx1+wMD2ifncAXUQE4SkDqN1hE0jEophs1SUwZAOhUAiMCLwRtamtTZtbbmZErSAUHbSysaoEmnrsakiMiUAURi283gN6wans9oX8rOCrj7/JP35DFD+iQ7Au/K2KE1jzx6ujjUnXFH9KjEq6ZlhsTBICrNLJf47Pv/pkHzvup1w4dmUbEei0+bcXRqJuh5kVARQ8byyYxOwNGr7A87xh1tp8sGT+uMInrwi++Xj7TQz2d27NvwEkrOflAFQGIDA5khASBCGdO2/Z/MnLPwYfv5TFhjW7QhVKAB6afwe2LpFlFsCnlQEosgQgDsdOG1/LKeNqJS4JCSPJ/i+TakwEARor7gER1Iva5JmPOJK0RUqmoPnnlzFCtmIAhAAQEIQRgDaiYPIauNXcnDlRIrWNFY3hm7PG9YRqr7IV7HrCgAC17befjEvRq2nGhAHtBqDpOuI/I1diUUAMYIxEdyejBJqLnNoszGZtfiX/CztGv2mq+sdaAAAAAElFTkSuQmCC"> | |||
<title>Lighthouse Report</title> | |||
<style>body {margin: 0}</style> |
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.
Can you add this to the flow template as well, or maybe put in shared styles somehow?
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.
Hmm looks like the report is rendering in the flow report
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.
Cool, thanks for fixing those!
following #12772, this PR adds a nice and minimal API for rendering the report. since earlier discussion the report API proposal has even shrank. We no longer need the
renderPartials()
approach we previously discussed.In this PR:
dom.rootEl
as the root DOM element of this report. Critical to have this for our PSI-like case of multiple reports in the same document. You'll spot a bunch of changes from_document
torootEl
in report-ui-features. All of those cases would otherwise be bugs.renderReport(lhr, opts)
API.omitTopbar
anddisableAutoDarkModeAndFireworks
options.onDetailsItemRendered
,onSaveGist
andgetStandaloneReportHTML
are not yet here, but those are next.