core(lhr): drop informative/manual, moving them to scoreDisplayMode #5105
Conversation
patrickhulce
requested review from
brendankenny,
paulirish and
vinamratasingal-zz
as code owners
patrickhulce
force-pushed
the
score_display_mode
branch
from
6eb070b
to
e2ac49c
Compare
| throw new Error(`${auditRef.id} accessibility audit does not have a group`); | ||
| } | ||
|
|
||
| if (auditRef.weight > 0 && isManual) { |
There was a problem hiding this comment.
I went ahead and did it here @brendankenny since I was rebasing anyway
yah sgtm |
| auditEl.classList.add('lh-audit--informative'); | ||
| } | ||
| if (audit.result.manual) { | ||
| if (audit.result.scoreDisplayMode === 'manual') { |
There was a problem hiding this comment.
lh-audit--${audit.result.scoreDisplayMode} cool with you?
paulirish
changed the title
|
travis sez run |
I think it makes sense, too. I could see a future where an audit doesn't have enough information and wants to mark itself informative, too. It makes sense for |
typings/audit.d.ts
Outdated
| @@ -114,12 +115,9 @@ declare global { | |||
| scoreDisplayMode: ScoringModeValue; | |||
| description: string; | |||
| extendedInfo?: {[p: string]: any}; | |||
| notApplicable?: boolean; | |||
There was a problem hiding this comment.
not sure where the best place to put docstrings for audit authors to see, but maybe add a jsdoc string to notApplicable in Product to mention it would override scoreDisplayMode if provided?
typings/audit.d.ts
Outdated
| } | ||
|
|
||
| export type ScoringModeValue = Audit.ScoringModes[keyof Audit.ScoringModes]; | ||
| export type ScoringModeValue = Audit.ScoreDisplayModes[keyof Audit.ScoreDisplayModes]; |
There was a problem hiding this comment.
should this type be ScoreDisplayMode now? Seems like it makes sense
| let {score, scoreDisplayMode} = Audit._normalizeAuditScore(audit, result); | ||
|
|
||
| // If the audit was determined to not apply to the page, we'll reset it as informative only | ||
| let informative = audit.meta.informative; | ||
| if (result.notApplicable) { | ||
| score = 1; |
There was a problem hiding this comment.
should we be enforcing the other things we say we do? Like no score if not binary or numeric display mode?
There was a problem hiding this comment.
yeah we're gonna do that in followup
|
@brendankenny LGTY? |
removes informative/manual/notApplicable booleans from
LH.Audit.Result, usesscoreDisplayModefor all of these nowkeeps
notApplicableon theLH.Audit.Productthough whichcreateAuditResultuses to set the scoreDisplayMode tonot-applicable, it felt a little more natural to me as the runtime split, but curious what other folks think@paulirish FYI this probably has a few implications for your design changes i.e. an audit can no longer be both binary informative (made some blue 0s show up)
ref #5008 #4333