Closed vrajashkr closed 8 months ago
Hi @andaaron , @rchincha , the PR is ready for review. Looking forward to your feedback and suggestions. Thank you!
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 82.87%. Comparing base (
33524ce
) to head (4e76918
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thank you @vrajashkr for the PR! In addition to the code comments. Design-wise I'm not necessarily the biggest fan of the extra nested card. I feel like maybe a section for each Package with the Package name as a heading would be cleaner imo. Also a point that we should consider regarding mobile view is if this additional information is something we consider useful in that scenario. We should just hide out package information that would not be useful for a mobile viewer Up to @rchincha on that point.
@raulkele , thanks a lot for the feedback! I'll address the review comments.
I feel like maybe a section for each Package with the Package name as a heading would be cleaner imo
This sounds like a good idea! I'll give it a try to see how it looks.
I made an attempt at trying out the sections as suggested. Here's what it looks like:
@raulkele, what do you think about this design?
IMHO the earlier implementation in the description looks better (but I'm not a front-end developer so I don't know what to say about how the code is written behind the scenes).
Personally, I'm torn between the two. I can't seem to settle on one 😅.
One key item here (at least in my opinion) appears to be the need to handle a variation in the length of the package name and the package path.
In both approaches, what stands out to me is that there's a lot of horizontal space that's not being used. For instance, the python vulnerability has no paths and the library name and versions are short so we could ideally have 3 cards in the same line on wide screens and they can automatically scale to 2 on a line or 1 per line on smaller mobile screens. However, when there's a long path like in the spring web one, it needs to take up the full width on its own.
The counter is that while this may lead to better use of horizontal space, it becomes somewhat inconsistent as some are multi per line and others are single per line depending on the package path length, so I'm not sure if it's such a good idea either.
Any thoughts @raulkele @andaaron ?
Design-wise I think I prefer the second implementation. I don't think it is an issue on larger screens that the row is empty if the path is not specified, we have examples where we do that in the ui.
I'll take a look at the code again as well if you update the PR or perhaps open up a second one with the other version.
One possible solution to the variance in the displayed data that you mentioned would be to make the row width dynamic. that way, depending on the amount of information, it would either be all three on one row or on separate rows as required. The downside of this would be that then the display would be inconsistent. So again we have to make a choice if we prefer using all the screen real-estate and sometimes be inconsistent, or accept that sometimes the amount of data leads to some free space.
Thanks for the inputs @raulkele ! I've updated this PR to address the review comments so it's green. Any further feedback is welcome!
For the section approach, I'll raise a new PR.
Design-wise I think I prefer the second implementation. I don't think it is an issue on larger screens that the row is empty if the path is not specified, we have examples where we do that in the ui.
Maybe hide those rows entirely? Do not show both "Package Path" and the value?
Maybe hide those rows entirely? Do not show both "Package Path" and the value?
One thought about this that I had was related to consistency. For "Fixed Version", ZUI shows "Not Specified". Displaying "Package Path" as "Not Specified" would help to keep it consistent so the user knows what to expect in this case.
We decided to use https://github.com/project-zot/zui/pull/428 instead.
What type of PR is this? feature
Which issue does this PR fix: Partially addresses https://github.com/project-zot/zot/issues/2175
What does this PR do / Why do we need it: This PR displays the Package Path information for the package list for a given CVE in the vulnerabilities list. Since there is more data being displayed, this PR also brings in a change to display this information in the form of a card that is vertically arranged.
Testing done on this change: Screenshots:
CSV export:
XLSX export:
Will this break upgrades or downgrades. Has updating a running cluster been tested?: Ideally, this should not break upgrades or downgrades as the older graphQL query should continue working just fine as well as the updated query. No, updating a running cluster has not been tested.
Does this change require updates to the CNI daemonset config files to work?: N/A
Does this PR introduce any user-facing change?: Yes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.