-
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
core(config): add new PWA category groups #6396
Conversation
delete manifest-short-name-length audit
removed descriptions (since not actually in the mocks), which makes this even easier :) |
{id: 'themed-omnibox', weight: 1}, | ||
{id: 'content-width', weight: 1}, | ||
{id: 'manifest-short-name-length', weight: 0}, | ||
// Fast and Reliable |
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.
hey this lines up exactly with "most difficult and critical for good UX" :D
do we need to re-order the lines? it lined up with "fast" and then "reliable" already ;)
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.
do we need to re-order the lines?
We don't need to, but since this will end up being a pretty big change in the html report, it seems better to have them in the same order
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.
oh, ha, nm, I see what you mean. SGTM :)
@@ -225,6 +224,15 @@ const defaultConfig = { | |||
title: str_(UIStrings.diagnosticsGroupTitle), | |||
description: str_(UIStrings.diagnosticsGroupDescription), | |||
}, | |||
'pwa-fast-reliable': { | |||
title: 'Fast and reliable', |
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.
could we start with these being str_
? :)
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.
could we start with these being str_ ?
good call, done
{id: 'webapp-install-banner', weight: 3, group: 'pwa-installable'}, | ||
|
||
// Engaging | ||
{id: 'redirects-http', weight: 2, group: 'pwa-engaging'}, // Arguably not engaging |
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.
with the new in-yo-face red X insecure I think it works, I'm always a bit taken aback on surge.sh URLs these days on mobile canary!
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.
with the new in-yo-face red X insecure I think it works
ha, fair point. I've removed the comment and anyone who objects can bring it up in the tracking bug :)
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.
lgtm % this:
need a slight tweak on PWA category description per the mocks.
'These checks validate the aspects of a Progressive Web App, as specified by the baseline [PWA Checklist](https://developers.google.com/web/progressive-web-apps/checklist).',
⬇️
'These checks validate the aspects of a Progressive Web App. [Learn more](https://developers.google.com/web/progressive-web-apps/checklist)',
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.
LGTM!
Starting on #6395
This PR just adds groups to the PWA category and deletes the manifest-short-name-length audit. Feel free to come up with better group descriptions or we can defer, I spent no time on them :)