-
-
Notifications
You must be signed in to change notification settings - Fork 167
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 finding detail and fix finding search routes #689
Conversation
@@ -11,6 +11,7 @@ module.exports = { | |||
proxy: { "/api": { target: process.env.VUE_APP_SERVER_URL} } | |||
}, | |||
configureWebpack: { | |||
devtool: 'source-map', |
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 is a drive-by improvement from the docs, which I used to make debugging easier.
e9d0705
to
0b2e876
Compare
2c199ca
to
d42e6a8
Compare
@@ -132,7 +132,7 @@ function configRoutes() { | |||
}, | |||
{ | |||
path: 'projects/:uuid/findings/:vulnerability', | |||
name: 'Project Finding Lookup', | |||
name: 'Project Vulnerability Lookup', |
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.
I noticed this existing name clash because vue-router was complaining about it in the console.
@@ -0,0 +1,247 @@ | |||
<template> |
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.
Signed-off-by: mykter <[email protected]>
Signed-off-by: mykter <[email protected]>
Signed-off-by: mykter <[email protected]>
Signed-off-by: mykter <[email protected]>
edfcded
to
cec631b
Compare
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.
Thanks @mykter, I really appreciate the attention to detail in this PR.
From laying out your reasoning in the description, to making sure each change is understandable one-by-one, this was a pleasure to look at.
Description
This PR fixes a pair of bugs and factors out the finding detail view into its own single file component.
It might be easiest to review commit by commit.
Addressed Issue
Fixes #688 in 3895307
Additional Details
I originally planned to make a new view to show a specific finding, and wanted to re-use the detail view from the audit page. This was inlined in a way that was very hard to re-use (or read!). In my journey to try and re-use it I came across Steve's earlier investigations, and was eventually able to pull it out into its own component as it looks like he originally wanted to.
I then discovered that there is already a route that shows this information, it just doesn't work.
I think the refactoring is still worthwhile by itself - maintaining this view in SFC form is easier than in the inline form. The refactoring allowed eslint to find some issues in the inlined code, for example.
Checklist