-
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: only print light theme #9173
Conversation
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.
nice this worked out nicely and super cleanly!
worth a test?
* @param {Function} cb | ||
*/ | ||
_setLightThemeTemporarily(cb) { | ||
const isDark = this._dom.find('.lh-vars', this._document).classList.contains('dark'); |
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.
yikes 😆 I wonder if we should use a variable or something in ReportUIFeatures state...
misread and realized this was testing classList
and not textContent
but a function or something that encapsulates this might be nice
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.
seems like what @media print
was made for? We couldn't do expansion that way because audit content is in <details>
elements, but seems like this would be possible?
How could a media rule unapply the is EDIT: yeah that works |
217b768 :P |
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.
all kidding aside, I do think @hoten 's old approach would be a great candidate for the expansion business to avoid mucking up the selection state!
Fixes #9155.