report(details-renderer): use new details types #7192
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
exterkamp
approved these changes
Feb 10, 2019
| @@ -43,52 +41,29 @@ class DetailsRenderer { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {DetailsJSON|OpportunityDetails} details | |||
| * @param {LH.Audit.Details} details | |||
There was a problem hiding this comment.
Add a comment for what this render function does. i.e. render the top level categories of details.
| @@ -100,7 +100,6 @@ declare global { | |||
| id: string; | |||
| /** A more detailed description that describes why the audit is important and links to Lighthouse documentation on the audit; markdown links supported. */ | |||
| description: string; | |||
| // TODO(bckenny): define details | |||
paulirish
approved these changes
Feb 14, 2019
|
|
||
| // Internal-only details, not for rendering. | ||
| case 'screenshot': | ||
| case 'diagnostic': | ||
| return null; | ||
| // Fallback for old LHRs, where no type meant don't render. | ||
| case undefined: | ||
| return null; |
There was a problem hiding this comment.
should we console.error or something?
There was a problem hiding this comment.
should we console.error or something?
I don't think it matters. This is for loading < 4.2 or whatever LHRs in the viewer. Things like diagnostics.js and metrics.js didn't have a type before, now they do (type: 'diagnostic'), so just need an explicit handling of the no-type type.
brendankenny
force-pushed
the
notypedefs
branch
from
5a5b159 to
cbb7c6c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uses the new audit details types from #7177 in
DetailsRenderer, finally deleting all those super old (and confusing) typedefs. Internal functionality is changed, but the generated DOM should be exactly the same.Majority of the PR is tests because
details-renderer-test.jswas really really light before this :)Main things were
opportunityandtablerendering (since they're both really tables). This required a_getCanonicalizedTableHeadingsmethod because they have differentheadingformats (which we can hopefully fix next breaking version).DetailsRenderer.render()switch statement reflecting theaudit-details.d.tstype structure that has a few rootdetailstypes, with all the common types (text,url, etc) within detailsitems. These are now rendered in_renderTableValue().As mentioned in #7177, there's nothing special about which types fall into
renderor_renderTableValue, this organization just keeps things narrowed to how we actually use these types. If at some point we needcodeto be directly used, it's as simple as addingLH.Audit.Details.Codeto theLH.Audit.Detailstype union and adding_renderCode()back to theDetailsRenderer.render()switch.