Skip to content
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

Finalize single CVE page layout #14177

Merged
merged 9 commits into from
Aug 23, 2024
Merged

Conversation

mtruj013
Copy link
Contributor

@mtruj013 mtruj013 commented Aug 14, 2024

Done

  • Addressed this list of needed fixes and implemented switch functionality for toggling unmaintained releases

QA

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-13137

@webteam-app
Copy link

@mtruj013 mtruj013 changed the base branch from cve-overhaul to main August 14, 2024 13:21
@mtruj013 mtruj013 changed the base branch from main to cve-overhaul August 14, 2024 13:21
@mtruj013 mtruj013 changed the title Single CVE Finalize single CVE layout Aug 14, 2024
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.57%. Comparing base (d9abba5) to head (46ca361).
Report is 204 commits behind head on cve-overhaul.

Additional details and impacted files
@@               Coverage Diff                @@
##           cve-overhaul   #14177      +/-   ##
================================================
- Coverage         69.64%   69.57%   -0.07%     
================================================
  Files               109      120      +11     
  Lines              3126     3412     +286     
  Branches           1089     1172      +83     
================================================
+ Hits               2177     2374     +197     
- Misses              924     1013      +89     
  Partials             25       25              

see 11 files with indirect coverage changes

@mtruj013 mtruj013 changed the title Finalize single CVE layout Finalize single CVE page layout Aug 14, 2024
@mtruj013
Copy link
Contributor Author

Ready for you @juanruitina ! Lmk what you think about the switch/notification functionality here https://ubuntu-com-14177.demos.haus/security/CVE-2011-1497, I had to guess as to how to handle a cve wherein none of the statuses are for maintained releases. Without a fallback for this (doesn't necessarily need to be this one) the user would land on the page and see an empty table unless they toggle the switch which doesn't seem super intuitive

@juanruitina
Copy link
Contributor

juanruitina commented Aug 14, 2024

Aaaaaa so cool. I think the fallback for CVEs that don't affect maintained releases is great, no changes there.

Some comments:

  • Remove the <div class="p-section--shallow"> div wrapping the Status heading, it's adding excessive padding.
  • Please sort the releases for each package in reverse chronological order
  • Make sure LTS are marked as such ("24.04 LTS", same as in the filters)
  • When "Show unmaintained releases" is unchecked, all supported releases should be visible, including the ones that require Pro (ESM). In other words, we should show the same ones that we do on the filters by default.
  • Please replace the "vulnerable" icon with p-icon--error, I agreed on that with Lyubo until we come up with new icons.
  • Please don't use icons for "ignored"
  • Tooltips are not implemented yet, I documented them here, reach out if you have questions.
  • Alignment is not great when text wraps in statuses, next line should be aligned to text, not to icon, example:

Screenshot 2024-08-14 at 16 28 26

  • In statuses, can we space icon, status and description a bit more? It should make them more discernible. Half a rem would do:

Screenshot 2024-08-14 at 16 22 18

  • When CVEs have a rationale for their priority (like this one), the "Why this priority?" link should be an anchor link pointing to the "Why is this CVE ??? priority?" section in the page. In that section, there should be a link to the Priority levels section in the About page: https://ubuntu-com-14177.demos.haus/security/cves/about#priority You can follow the designs for this:

Screenshot 2024-08-14 at 16 42 33

@mtruj013
Copy link
Contributor Author

Thanks @juanruitina, could you take another look?

@juanruitina
Copy link
Contributor

juanruitina commented Aug 21, 2024

Some minor stuff on responsiveness:

  • Horizontal scrolling isn't working in the status table, can you check?
  • Shall we maybe make columns equal in the "Severity score breakdown" table on mobile? It looks unbalanced:

Screenshot 2024-08-21 at 16 55 50

  • USN also don't wrap great. It's a pity we don't use display: flex; flex-wrap: wrap; for p-inline-list instead of inlining children, such headache.

Screenshot 2024-08-21 at 16 57 49

  • In the tooltips: can we get rid of the weird gap in the second line?

Screenshot 2024-08-21 at 17 00 49

There's also something that I don't really like: the "Ubuntu Pro" chips are links, which is semantically correct, but Vanilla adds an underline on hover, and doesn't show the chip's hover styles that it shows when they are buttons. I've submitted it as a bug in the Vanilla repo: canonical/vanilla-framework#5317, it's not a blocker for this though.

@juanruitina
Copy link
Contributor

Thank you for making the many changes, nice work! LGTM

@immortalcodes
Copy link
Member

LGTM! Just small issues with the side navigation not highlighting the current heading.
Please fix the checks and it would be good to go.

@mtruj013 mtruj013 merged commit 48cde6d into canonical:cve-overhaul Aug 23, 2024
14 of 15 checks passed
@mtruj013 mtruj013 deleted the single-cve branch August 23, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants