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

EOL notice #193

Merged
merged 2 commits into from
Jan 9, 2025
Merged

EOL notice #193

merged 2 commits into from
Jan 9, 2025

Conversation

rviscomi
Copy link
Member

@rviscomi rviscomi commented Dec 4, 2024

See https://developer.chrome.com/blog/web-vitals-extension

  • add notice to the README
  • add notice to the console log, when enabled
  • add notice to the popup, with a persistent option to stop showing it
  • link to the blog post in CrUX error logs
image

@rviscomi rviscomi marked this pull request as ready for review January 9, 2025 16:19
Copy link
Member

@tunetheweb tunetheweb left a comment

Choose a reason for hiding this comment

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

Haven't tested it but change LGTM.

Very thoughtful to add a dismiss option, and to persist it to settings!!

@@ -11,7 +11,6 @@ export class CrUX {
const origin = urlHelper.origin;

return CrUX.query({url, formFactor}).catch(e =>{
console.warn('CrUX URL data unavailable', e);
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why we have this here as well as further down?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it seemed redundant to me

src/browser_action/vitals.js Outdated Show resolved Hide resolved
Copy link
Member

@addyosmani addyosmani left a comment

Choose a reason for hiding this comment

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

LGTM. I've been supportive of the EOL for the extension and our shift to encourage just using DevTools directly given most of the features we ported over. Thanks for your work on this, @rviscomi! 🤝

@addyosmani addyosmani merged commit 5071422 into main Jan 9, 2025
4 checks passed
@addyosmani addyosmani deleted the eol-notice branch January 9, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants