-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: add tooltip to qualify stackpack recommendations #14913
base: main
Are you sure you want to change the base?
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.
IMO there is a very low chance that anyone notices this. A prefix line explaining that these stacks were detected on the page already would be better.
I understand the clutter argument, but I think there are ways of resolving clutter without basically hiding this message (increase spacing, outlined stack packs box, smaller font, etc.)
@@ -92,6 +92,7 @@ export class CategoryRenderer { | |||
const snippets = this.dom.convertMarkdownLinkSnippets(pack.description); | |||
const packElm = this.dom.createElement('div', 'lh-audit__stackpack'); | |||
packElm.append(packElmImg, snippets); | |||
packElm.title = `Recommendation specific to ${pack.title} detected on the page`; |
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.
We should create a UI string for this regardless of what UI direction we go in:
https://github.com/GoogleChrome/lighthouse/blob/main/report/renderer/report-utils.js#L350
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.
Yeah I was looking at it just now and I agree; we've one more string at L82 that isn't i18ned.
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.
Ah, I think the reason that isn't formatted is because the strings can change based on runtime values. We would need to call formatMessage
here to translate this.
The flow report already does this in a few places but the normal report renderer does not:
https://github.com/GoogleChrome/lighthouse/blob/main/flow-report/src/i18n/i18n.tsx#L50
@adamraine @alexnj can we merge or close this? |
I'll leave it up to @alexnj but creating a UX that makes everyone happy could be more work than it's worth. |
Addresses #14894 by adding a tooltip to StackPack recommendations.
Example: