Repository navigation
Search result bookcover image space reservation - #13129
Conversation
|
Thanks for the PR, @lbartron! 🤖 Copilot has been assigned for an initial review. @lokesh is assigned to this PR and currently has:
Possible improvements for this PR
PR triage checklist (maintainers / Richy)
Note This comment was automatically generated by PAM, Open Library's Project AI Manager. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting. |
There was a problem hiding this comment.
Pull request overview
Reserves layout space for cover images in list-view search results to reduce cumulative layout shift (CLS) by using per-book cover dimensions (cover_width/cover_height) from Solr, with a 2/3 fallback ratio when dimensions are missing.
Changes:
- Fetch
cover_width/cover_heightin the worksearch Solr scheme defaults. - Pass
cover_width/cover_heightthroughget_doc()so templates can access them. - Apply
aspect-ratioin CSS and (when available) set--cw/--chfrom the selected edition in the search result template.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| static/css/components/search-result-item.css | Switches cover rendering to aspect-ratio (with CSS var fallback) and height: auto for stable layout. |
| openlibrary/plugins/worksearch/schemes/works.py | Requests cover_width/cover_height from Solr as available fields and default fetched fields. |
| openlibrary/plugins/worksearch/code.py | Exposes cover_width/cover_height on the doc object returned to templates. |
| openlibrary/macros/SearchResultsWork.html | Sets per-result CSS variables (--cw/--ch) from the selected edition when available. |
| first_edition=doc.get("first_edition", None), | ||
| subtitle=doc.get("subtitle", None), | ||
| cover_edition_key=doc.get("cover_edition_key", None), | ||
| cover_width=doc.get("cover_width", None), | ||
| cover_height=doc.get("cover_height", None), |
| $ cover = get_cover_url(selected_ed) or "/static/images/icons/avatar_book-sm.png" | ||
| $ cover_w = selected_ed.get('cover_width') | ||
| $ cover_h = selected_ed.get('cover_height') | ||
| <a href="$work_edition_url"> | ||
| <img | ||
| itemprop="image" | ||
| src="$cover" | ||
| $:cond(cover_w and cover_h, 'style="--cw:%s;--ch:%s;"' % (cover_w, cover_h)) |
| .searchResultItem .bookcover img { | ||
| border-radius: var(--border-radius-thumbnail); | ||
| max-width: 100%; | ||
| max-height: 300px; | ||
| width: 175px; | ||
| height: auto; | ||
| aspect-ratio: var(--cw, 2) / var(--ch, 3); | ||
| } |
|
Hello! This pull request has been marked as stale because it has been waiting for submitter input for 7 days. If you're still working on this, please add a comment to keep it open. Otherwise, a maintainer will have to determine how to proceed including reassigning or opening the issue for others to work on after communicating with you. |
|
Hello! This pull request has been marked as stale because it has been waiting for submitter input for 7 days. If you're still working on this, please add a comment to keep it open. Otherwise, a maintainer will have to determine how to proceed including reassigning or opening the issue for others to work on after communicating with you. |
|
Pushed a couple of commits to this branch — merged master and reworked the space reservation. Summary of what was wrong and what changed:
Branch was stale. The CSS Swapped the CSS custom properties for plain Added a scheme test asserting the fields reach both the work One thing I left alone: the |
They only need to be in default_fetched_fields to come back from Solr; all_fields is the user-searchable set, and cover_width:[600 TO *] isn't query syntax we want to expose.
The no-cover avatar is 84x115, not 2:3, so the 180x270 fallback reserved 262px and settled at 240px on load. Take the dimensions from the src we actually chose, and let the placeholder win over stale Solr dimensions when no cover URL resolves.
|
Following up on point 3 from my earlier comment — I dug into it and it's more structural than I first thought, so writing it down before it gets lost. The dimensions and the Nothing looks broken when it happens, since Patching a single branch won't hold — the other four can still drift. The durable version is to have the resolver answer both questions at once: a I'd keep that out of this PR — it touches Two smaller things on the current branch, both of which I'm taking care of so you don't have to: the Still want screenshots and a CLS measurement before this merges. One heads up there: local covers won't tell you anything useful, since the dev coverstore serves blank 1×1 placeholders that report a 1:1 ratio and mask the shift. Point covers at the production CDN first, and make sure your sample includes a coverless result and a lending/availability result so both of the above show up in the numbers. |
19aefc8 to
6146baa
Compare
get_cover_url prefers the availability edition or IA cover over the work's cover_i, so Solr's cover_width/cover_height can describe a different image. Fall back to 2:3 in that case.
…cover-image-space # Conflicts: # openlibrary/plugins/worksearch/schemes/tests/test_works.py
lokesh
left a comment
There was a problem hiding this comment.
LGTM. Tested locally.
A few changes I made to make it more bulletproof:
- Brought back the 300px height limit that had been removed, so very tall covers can't make rows huge.
- Added object-fit: contain so covers shrink to fit instead of getting squashed.
- Used Solr's numbers only when they describe the cover actually shown. Otherwise it falls back to a standard 2:3 shape.
Closes #12825
Reserves layout space for cover images in list-view search results to prevent cumulative layout shift. I originally suggested a flat aspect ratio of 180 / 270. This PR uses the actual per-book
cover_widthandcover_heightfields that are now available in Solr thanks to PR #12916.The
<img>getswidth/heightattributes so the browser reserves the right box before the cover loads. When dimensions aren't available (likely for many books, since the Solr fields are recent), or when the cover shown isn't the one those dimensions describe, it falls back to 180×270 (2:3). Results with no cover use the placeholder's own 84×115.Technical
openlibrary/plugins/worksearch/schemes/works.py: requestcover_width/cover_heightfrom Solr viadefault_fetched_fields, which also carries them into theeditions.flsubquery.openlibrary/plugins/worksearch/code.py: add both fields toget_doc()'s allow-list so they reach the template.openlibrary/macros/SearchResultsWork.html: setwidth/heighton the cover<img>. Solr dimensions are only used when the displayed URL was built from the doc'scover_i. For a work,get_cover_url()prefers the availability edition's cover or the IA cover overcover_i, so the indexed dimensions would describe a different image.static/css/components/search-result-item.css: addheight: auto; object-fit: containto.searchResultItem .bookcover imgso the attributes reserve space and the real ratio wins once the image loads. The existingwidth: 175px/max-height: 300pxconstraints are unchanged. Noaspect-ratiois declared, since it would override the image's real ratio.Testing
test_cover_dimensions_fetched_for_works_and_editionschecks the fields are indefault_fetched_fieldsand ineditions.fl.test_get_doccovers the new fields passing throughget_doc().Screenshot
To follow with the manual verification.
Stakeholders
@lokesh