core: change 'not-applicable' to 'notApplicable' #6783
Conversation
exterkamp
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
| @@ -124,7 +124,7 @@ declare global { | |||
| warnings?: string[]; | |||
| score?: number; | |||
| extendedInfo?: {[p: string]: any}; | |||
| /** Overrides scoreDisplayMode with not-applicable if set to true */ | |||
| /** Overrides scoreDisplayMode with notApplicable if set to true */ | |||
| notApplicable?: boolean; | |||
There was a problem hiding this comment.
a head of its time 😆
| @@ -56,7 +56,7 @@ class CategoryRenderer { | |||
| title: Util.UIStrings.passedAuditsGroupTitle, | |||
| className: 'lh-clump--passed', | |||
| }, | |||
| 'not-applicable': { | |||
| 'notApplicable': { | |||
| title: Util.UIStrings.notApplicableAuditsGroupTitle, | |||
| className: 'lh-clump--not-applicable', | |||
There was a problem hiding this comment.
oh we should get a puppeteer screenshot test in for the report to catch these css mismatches
There was a problem hiding this comment.
oh we should get a puppeteer screenshot test in for the report to catch these css mismatches
good idea. Don't we also have a jsdom test or two looking for the not applicable results? Concerning they weren't found...
There was a problem hiding this comment.
Concerning they weren't found...
oh, because the test selectors weren't updated either :)
But now I'm questioning if it should be updated :) We don't currently create the clump names dynamically, so it can be whatever we want. Commonly notApplicable in js goes to not-applicable in css, but do we care? Or is notapplicable fine? Keeping camelCase seems gross, though.
There was a problem hiding this comment.
I would go for not-applicable makes it easier to read in my opinion
| @@ -124,7 +124,7 @@ declare global { | |||
| warnings?: string[]; | |||
| score?: number; | |||
| extendedInfo?: {[p: string]: any}; | |||
| /** Overrides scoreDisplayMode with not-applicable if set to true */ | |||
There was a problem hiding this comment.
also should be notApplicable in the comment ~20 lines below here
…audit class to .lh-audit--notapplicable
…in other tests. Added missing SDM test.
| @@ -153,7 +153,10 @@ class CategoryRenderer { | |||
| */ | |||
| _setRatingClass(element, score, scoreDisplayMode) { | |||
| const rating = Util.calculateRating(score, scoreDisplayMode); | |||
| element.classList.add(`lh-audit--${rating}`, `lh-audit--${scoreDisplayMode}`); | |||
| element.classList.add(`lh-audit--${rating}`); | |||
| if (typeof scoreDisplayMode === 'string') { | |||
There was a problem hiding this comment.
wait...how would it be possible for it not to be? That seems bad if it happens. Just in tests maybe (hopefully)?
There was a problem hiding this comment.
from https://travis-ci.org/GoogleChrome/lighthouse/builds/470305380#L816, it looks like just the unit tests. We should fix those (is that what fdf9755 did?) and then not change the code here, I think. Better to trust our type system and fail fast if we defined/enforced things poorly.
There was a problem hiding this comment.
Yeah fdf9755 fixed the unit tests and added guard rails for if the scoreDisplayMode was undefined. Good point tho, I removed the guardrails and made all the tests actually represent reality!
| @@ -78,7 +78,7 @@ describe('processing for proto', () => { | |||
| const input = { | |||
| 'audits': { | |||
| 'critical-request-chains': { | |||
| 'scoreDisplayMode': 'not-applicable', | |||
| 'scoreDisplayMode': 'notApplicable', | |||
There was a problem hiding this comment.
ha, sorry, this one we want
There was a problem hiding this comment.
delete not-applicable
don't delete not-applicable
I just can't impress you brendan 😭
|
💥 |
Summary
Change not-applicable to notApplicable. This will allow the LHR and protoLHR to use to same value instead of converting it in preprocessing.
Unfortunately since we can't ignore old LHRs that use 'not-applicable' or 'not_applicable' the processors, proto, and renderer need to accommodate those values.
Related Issues/PRs
fixes: #6201