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

Search results page, layout and pagination #13440

Merged
merged 11 commits into from
Jan 12, 2024

Conversation

mtruj013
Copy link
Contributor

@mtruj013 mtruj013 commented Jan 8, 2024

Done

  • Applied design updated to search results page
  • Updated pagination functionality
  • Added ordering functionality
    Note: Filtering functionality to be covered in separate PR

QA

  • View the site locally in your web browser at: https://ubuntu-com-13440.demos.haus/security/cves
  • Click on any of the "By Ubuntu release" links and check results page against design
    • Be sure to test on mobile, tablet and desktop screen sizes
  • Change sorting option and see that you are returned the expected order
  • Test pagination, change the result limit and see that this works as expected
  • Click on the "Export to JSON" link and see that you can access the correct api endpoint url

Issue / Card

Fixes #https://warthogs.atlassian.net/browse/WD-5649

@webteam-app
Copy link

Demo starting at https://ubuntu-com-13440.demos.haus

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea68126) 74.41% compared to head (d8e0b84) 74.41%.

Additional details and impacted files
@@              Coverage Diff              @@
##           cve-overhaul   #13440   +/-   ##
=============================================
  Coverage         74.41%   74.41%           
=============================================
  Files               107      107           
  Lines              2838     2838           
  Branches            946      946           
=============================================
  Hits               2112     2112           
  Misses              702      702           
  Partials             24       24           

@mtruj013 mtruj013 changed the title WIP - Search results page, layout and pagination Search results page, layout and pagination Jan 8, 2024
@juanruitina
Copy link
Contributor

juanruitina commented Jan 8, 2024

Aaaaaah I'm loving this 💫 Some comments:

  • Please make CVE numbers/titles H2s (keep the styling).
  • Please make the description 3 lines max.
  • On the search results "header" ("1 – 10 of 841 results") and footer (pagination): I see you used columns here, but both left and right will likely change in width, and particularly the stuff on the right might take more than 3 columns (in fact, it already does when selecting "Newest"). I think using flexbox with justify-content: space-between here could be more appropriate, will make it more flexible and responsive. It would also make the stuff on the right align automatically.
  • Change "Sort by" in the footer with "Results by page" as per the design.
  • Add some spacing between "Detailed view" and "Sort by", and between "Export to JSON" and "Results by page".
  • Please drop the "and XX more" after the list of affected packages, it's awkward next when we already give a count just above. Just "…" will do. My fault, this was inconsistent across designs.

(I see "Export to JSON" still doesn't link to the API endpoint)

This is filters-related but sharing now given you are asking about responsiveness: filters should always be accessible one way or another. For medium and small, please use a toggle as we did here; I've adjusted the mobile design to account for this. I'll hold further feedback on the filters ✌️

Unrelated: I've noticed some CVEs don't show the summarised status, neither here nor in the home.

@@ -0,0 +1,46 @@
<section class="p-section">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was already reviewed and the only change is that I moved it into a partial to make the base page a bit more readable, so no need to review this one

templates/security/cve/_cve-landing.html Outdated Show resolved Hide resolved
webapp/security/helpers.py Show resolved Hide resolved
@mtruj013
Copy link
Contributor Author

@juanruitina ready for you once more, with a couple caveats:

  1. The summarized statuses bug turned out to be a thorny one. I need more time to work through it, but I'm going to create a separate pr as this one has already ballooned quite a bit.
  2. I was going to add the toggle filters button to this pr, but thought better of it for the same reason as above. So I'll follow up with a separate pr for that as well.

Besides that lmk if there's anything I missed!

@juanruitina
Copy link
Contributor

juanruitina commented Jan 12, 2024

Some minor things:

  • Please hide the "Detailed view" switch on mobile, it won't be available there.
  • Both header and footer don't wrap on mobile, the footer looks particularly bad. flex-wrap: wrap should do the trick.
  • The search button looks weird on mobile and doesn't match the design. I see some inline styles (margin-top: 2.6rem; width: 100%;), maybe you can apply these only for medium up?

I'll ignore filtering and summarised statuses for now. Feel free to change the label when this is fixed. Thanks a bunch!

templates/security/cve/_cve-card.html Outdated Show resolved Hide resolved
templates/security/cve/_cve-card.html Outdated Show resolved Hide resolved
templates/security/cve/_cve-card.html Outdated Show resolved Hide resolved
templates/security/cve/_cve-filters.html Outdated Show resolved Hide resolved
templates/security/cve/_cve-landing.html Outdated Show resolved Hide resolved
templates/security/cve/_cve-search-results.html Outdated Show resolved Hide resolved
templates/security/cve/_pagination.html Show resolved Hide resolved
webapp/security/helpers.py Show resolved Hide resolved
webapp/security/views.py Show resolved Hide resolved
webapp/security/views.py Show resolved Hide resolved
Copy link
Contributor

@akbarkz akbarkz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mtruj013 mtruj013 merged commit a83094c into canonical:cve-overhaul Jan 12, 2024
15 checks passed
@mtruj013 mtruj013 deleted the search-results-layout branch January 12, 2024 13:39
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