Skip to content

WIP: report: configure features#12327

Closed
connorjclark wants to merge 2 commits into
masterfrom
report-features-options
Closed

WIP: report: configure features#12327
connorjclark wants to merge 2 commits into
masterfrom
report-features-options

Conversation

@connorjclark

@connorjclark connorjclark commented Apr 2, 2021

Copy link
Copy Markdown
Collaborator

Configuration might be useful?

no impact on the devtools integration (it overrides some printing stuff which probably shouldn't be configurable)

the PSI integration would benefit slightly

@google-cla google-cla Bot added the cla: yes label Apr 2, 2021

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this file was ESM, then we could just export the setupElemScreenshots method? would that be equivalent?


// Do not query the system preferences for DevTools - DevTools should only apply dark theme
// if dark is selected in the settings panel.
if (!this._dom.isDevTools() && window.matchMedia('(prefers-color-scheme: dark)').matches) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the idea that a devtools rendering config would have toggleDarkMode: false (instead of this isDevTools()) ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didnt consider that but ya, sure

this._document.addEventListener('keyup', this.onKeyUp);
this._document.addEventListener('copy', this.onCopy);
if (this.featureSet.mediaQueryListeners) {
this._setupMediaQueryListeners();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

driveby: this fn should be renamed to applyNarrowClassSometimes or something

@connorjclark

Copy link
Copy Markdown
Collaborator Author

if this file was ESM, then we could just export the setupElemScreenshots method? would that be equivalent?

Ignoring the unsolved problem of us using ESM... I suppose? Although I prefer this method (shoving the entire interface into an options object) vs. making arbitrary methods "public".

@brendankenny

Copy link
Copy Markdown
Contributor
/**
 * @typedef FeatureSet
 * @property {boolean} dropDownMenu
 * @property {boolean | {containerEl: Element}} elementScreenshotOverlay
 * @property {boolean} fireworks
 * @property {boolean} i18n
 * @property {boolean} mediaQueryListeners
 * @property {boolean} printing
 * @property {boolean} showMetricDescriptionsOnError
 * @property {boolean} stickyHeader
 * @property {boolean} thirdPartyFilter
 * @property {boolean} toggleDarkMode
 */

what of these features is PSI likely to end up using?

@connorjclark

Copy link
Copy Markdown
Collaborator Author

I think these would be desirable:

  • elementScreenshotOverlay (already in effect)
  • showMetricDescriptionsOnError
  • thirdPartyFilter

Fireworks would be cool, but understandable to avoid :)

@exterkamp wdyt?

@connorjclark

Copy link
Copy Markdown
Collaborator Author

Shane agreed with the three features I listed above.

@connorjclark

Copy link
Copy Markdown
Collaborator Author

also: should move this._saveGistCallback from Viewer features to base features, so that DevTools can use it (if we figure out how to postMessage...)

@brendankenny

brendankenny commented May 18, 2021

Copy link
Copy Markdown
Contributor

the download, opening, then saving is unfortunate, but saving a gist requires having github sign in, so I'm not sure if that's ever going to work in devtools

@connorjclark

Copy link
Copy Markdown
Collaborator Author

closing for #12254

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants