Skip to content
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

WIP: report: configure features #12327

Closed
wants to merge 2 commits into from
Closed

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Apr 2, 2021

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
Copy link
Member

@paulirish paulirish left a comment

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
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
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
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
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
Member

/**
 * @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
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
Collaborator Author

Shane agreed with the three features I listed above.

@connorjclark
Copy link
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
Copy link
Member

brendankenny commented May 18, 2021

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
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.

None yet

3 participants