report: add options disableFireworks and disableDarkMode #13649
Conversation
report/clients/standalone.js
Outdated
|
|
||
| // Fireworks look better in dark mode. | ||
| if (reportRootEl.querySelector('.lh-score100')) { | ||
| toggleDarkTheme(new DOM(document, reportRootEl), true); |
There was a problem hiding this comment.
I like the idea of abstracting DOM behind renderReport. IMO it would be better to have a enableDarkModeWithFireworks option and moving this logic back to ReportUIFeatures. We could also use it for the report viewer.
There is also an argument to remove this feature, it can be unexpected for some users #13631.
There was a problem hiding this comment.
It was unexpected because there were no fireworks. With fireworks, there is context, and it is clearly an easter egg.
I like the idea of abstracting DOM behind renderReport.
If only the standalone report needs this, I'm fine keeping this internal.
There was a problem hiding this comment.
If only the standalone report needs this, I'm fine keeping this internal.
What about report viewer?
There was a problem hiding this comment.
hmmm forgot about that... ok how's the new changeset?
connorjclark
changed the title
Deciding to use the dark theme should be left to the embedder, in case it wouldn't mesh with the site theme. We tried to do this with
disableAutoDarkModeAndFireworksbut:This PR:
disableAutoDarkModeAndFireworksdisableDarkModeoption (embedders such as DevTools can set this and defer to the DevTools theme system to toggle the dark theme; or they can set it because the site should never be displayed in a dark theme and having just LH be dark would be jarring)disableFireworksoption