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

report: render notApplicable metrics with double dash #13981

Merged
merged 1 commit into from
May 10, 2022
Merged

Conversation

brendankenny
Copy link
Member

part of #13916

@adamraine convinced me in that issue that we should just use scoreDisplayMode notApplicable to allow metrics to declare themselves not-applicable, but (unlike all other audits) still be displayed in the report. This allows for "optional" metrics like responsiveness to correctly appear in timespan reports, even if there were no interactions in that timespan.

I still think there's a chance we won't want to eliminate the only way audits can dynamically remove themselves from the report, but that chance seems very slim since we've never done this with a metric before in the four years since metric rendering was changed much, and I think we'll still be able to change our minds without really breaking backwards/forwards compatibility with report rendering.

All that said: how should this look? Almost everything was already working so this PR is just adding some placeholder text. It's pretty weak right now, open to ideas :)

lighthouse report metrics with '--' in the place of the missing 'Interaction to next paint' value

  • better styling to offset it slightly?
  • @paulirish had proposed having custom text there (e.g. "No interactions in timespan" or something for INP), which is possible, since notApplicable audits can still have their displayValue set and could set their own string there. Would that be better? Would need to make sure the string (and translations) fit.

@brendankenny brendankenny requested a review from a team as a code owner May 9, 2022 20:20
@brendankenny brendankenny requested review from connorjclark and removed request for a team May 9, 2022 20:20
@brendankenny
Copy link
Member Author

LHR for anyone who wants to bikeshed: https://gist.github.com/6be296a3c582d36e850ae1d102270743

@connorjclark
Copy link
Collaborator

I'd prefer N/A here, even though we'd need to translate it (we could mention to the translators if they can't keep it short, then just provide --). but don't have any strong opinions on this.

@adamraine
Copy link
Member

adamraine commented May 9, 2022

better styling to offset it slightly?

Could it be gray like other N/A stuff

@paulirish had proposed having custom text there (e.g. "No interactions in timespan" or something for INP), which is possible, since notApplicable audits can still have their displayValue set and could set their own string there. Would that be better? Would need to make sure the string (and translations) fit.

I like the idea of using display value. Just doing "N/A" or "--" doesn't really explain why the metric is absent. Could use "--" or "N/A" as a fallback if display value is falsy?

@connorjclark
Copy link
Collaborator

double dash is good for now, we can bikeshed this more post 9.6
image

@connorjclark connorjclark changed the title report: render notApplicable metrics report: render notApplicable metrics with double dash May 9, 2022
@connorjclark connorjclark merged commit 62bd578 into master May 10, 2022
@connorjclark connorjclark deleted the na-metric branch May 10, 2022 00:04
@connorjclark
Copy link
Collaborator

wtf why didn't the commit take the PR title

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

Successfully merging this pull request may close these issues.

4 participants