-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
refactor: split report-renderer into multiple files #1987
Conversation
works for me |
Works for me too. You know I love that |
@patrickhulce you good on this? I don't want to get too deep and have rebase nightmares. |
yep I'm ready if you are! |
module.exports = DetailsRenderer; | ||
} | ||
|
||
/** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this won't work (it's scoped to the module like regular variables). I believe you can put it on DetailsRenderer
, but then it'll have to be referred to as DetailsRenderer.DetailsJSON
in report-renderer.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although you can wait on that, because these are broken anyways (no non-null and I believe union types need parens when nested like this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reorg LGTM
In preparation for introducing tags/grouped audit renderers the report-renderer.js started getting unwieldy. This splits it into 3 files DOM, DetailsRenderer, and ReportRenderer.
I know @brendankenny had Eric kill DOM but c'mon having these duplicated everywhere is bad and the class allows for easy test injection. I'll split up the test files too, but currently it conflicts with @ebidel's PR and figured I'd wait before I get too far.