report: augment auditRef, not result, with stackPack ref #8633
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
brendankenny
commented
Apr 26, 2019
| @@ -72,21 +72,21 @@ class Util { | |||
| } | |||
| } | |||
|
|
|||
| // For convenience, smoosh all AuditResults into their auditDfn (which has just weight & group) | |||
| // For convenience, smoosh all AuditResults into their auditRef (which has just weight & group) | |||
There was a problem hiding this comment.
change this and "auditMeta" below to their modern names :)
patrickhulce
approved these changes
Apr 26, 2019
| @@ -81,8 +81,8 @@ class CategoryRenderer { | |||
| this.dom.find('.lh-audit__description', auditEl) | |||
| .appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description)); | |||
|
|
|||
| if (audit.result.stackPacks) { | |||
| audit.result.stackPacks.forEach(pack => { | |||
| if (audit.stackPacks) { | |||
There was a problem hiding this comment.
this makes more sense to me 👍
| const clonedSampleResult = JSON.parse(JSON.stringify(sampleResult)); | ||
| const iconDataURL = 'data:image/svg+xml,%3Csvg xmlns="http://www.w3.org/2000/svg"%3E%3C/svg%3E'; | ||
| clonedSampleResult.stackPacks = [{ | ||
| id: 'snackpack', |
exterkamp
approved these changes
Apr 26, 2019
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.
Not sure if I can convince everyone we should make this change :) Note that it's contained entirely within the report renderer, so there will be no effect on core, PSI, etc.
In
prepareReportResultwe smoosh a few things to make report rendering easier, like writing aLH.Audit.Resultto the relevantLH.Result.AuditRefand, most recently, writingLH.ReportResult.StackPackDescriptionto the relevantLH.Audit.Result.This PR makes it so both
LH.Audit.ResultandLH.ReportResult.StackPackDescriptionare written to the auditRef.On the con side: it makes a certain amount of sense to match stack packs to auditResults as they have a 1:1 match, while there may be multiple auditRefs to a single audit (and thus a stack pack).
However:
The solution in this PR keeps modifications contained within
LH.ReportResult.AuditRefand makes that deletion in #8536 unnecessary.(@exterkamp if we land this you won't need any of your changes to
util-test.js)