Using book cover sizes to prevent layout shifting #10145

anna-ayn · 2024-12-13T03:19:26Z

Closes #9739

feature

Technical

Improvements to cover image handling:

openlibrary/book_providers.py: Added the get_dimensions_cover function to retrieve the dimensions of cover images from a given URL.
openlibrary/core/models.py: Added width and height methods to the Image class to get the dimensions of an image.

Updates to CSS styles:

openlibrary/components/LibraryExplorer/components/OLCarousel.vue: Added styles for the .cover-container class to handle different screen sizes and ensure images fit properly.
static/css/components/book.less: Changed img.bookcover to use filter: drop-shadow instead of box-shadow and ensured the width is set to 100%.
static/css/components/listLists.less: Adjusted the width of images in lists to 22px.

Template modifications:

openlibrary/macros/SearchResultsWork.html: Incorporated the get_dimensions_cover function to dynamically set the aspect ratio and size of cover images.
openlibrary/templates/books/custom_carousel.html: Apply the aspect ratio for book covers in the carousel.
openlibrary/templates/covers/book_cover.html: Added logic to calculate the height based on the aspect ratio and fixed width for book covers.
openlibrary/templates/lists/home.html: Added logic to calculate the height based on the aspect ratio and fixed width for book covers in the section of Active Lists.
openlibrary/templates/books/edition-sort.html: Apply the aspect ratio for book covers in Edition table.

Testing

Navigate to pages where book covers are displayed:

http://localhost:8080/search?q=mark&mode=everything
http://localhost:8080/works/OL13101191W/Alice%27s_Adventures_in_Wonderland?edition=key%3A/books/OL23269118M
http://localhost:8080/books/OL24375501M/Program_or_be_Programmed
http://localhost:8080/authors/OL22098A/Lewis_Carroll
Create a new list and add one or more books to the list: http://localhost:8080/people/openlibrary/lists/OL1L/new_list
http://localhost:8080/lists: Maybe, you will need to modify the code of the function _get_active_lists_in_random(limit=20, preload=True) as follows:

This is to show all the lists in the "Active Lists" section if you have few lists in your database.

Verified that the book covers load with the correct dimensions.
Ensured that the layout remains stable and does not shift when the images are loaded.
Tested the changes on various devices and screen sizes to confirm responsiveness.

Screenshot

We have make videos demonstrating the changes and comparing with the real website of OpenLibrary;
https://drive.google.com/file/d/1RrzodyWAFDAS--mk5Y222Hjk95FYgMla/view?usp=sharing
https://drive.google.com/file/d/1BF7YWPcys6Ny2EuJ9ahkiIexsC1JjmCH/view?usp=sharing
https://drive.google.com/file/d/1UU0nRT32Oa0Bu_27FD6cN8tQES-QUqpQ/view?usp=sharing
https://drive.google.com/file/d/1qe68bcZtBNh2cbOEbi93z4s2u9XHgDtD/view?usp=sharing
https://drive.google.com/file/d/1rOheT5PNJi6iveGhcGpb589YsG9uEqXb/view?usp=drive_link

Stakeholders

@cdrini

anna-ayn · 2024-12-14T00:49:15Z

Hi @RayBB and @cdrini,

We have completed the modifications to standardize the sizes of book covers and prevent layout shifting in various sections of the site.

We would greatly appreciate your review and feedback on these changes. Thank you!

mekarpeles · 2024-12-16T23:52:33Z

Can you provide some screenshots so we can visually inspect the before/after? Thank you!

EDIT: I now see the videos! Disregard :)

Also, it seems like the commit history may need a bit of a rebase / cleanup.

@RayBB do you mind taking a first look?

…te of lists Remove docs for calculating cover image dimensions in home template

…adjust image height to covers in already-lists

fix trailing spaces Remove unnecessary blank lines in initMyBooksAffordances function

A new container is added and the size of the covers is standardized A new container is added and the size of the covers is standardized

…static/css/components/search-result-item.less file. applied black formatter

anna-ayn · 2024-12-17T01:50:12Z

Hello @mekarpeles
No problem! We're glad you found the videos.

Regarding the commit history, we just done a rebase and cleanup.

Feel free to reach out if you have any other questions or need additional information!

codecov-commenter · 2024-12-17T01:54:51Z

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 17.42%. Comparing base (347bff9) to head (8b388aa).
Report is 173 commits behind head on master.

Files with missing lines	Patch %	Lines
...enlibrary/plugins/openlibrary/js/my-books/index.js	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10145      +/-   ##
==========================================
+ Coverage   17.12%   17.42%   +0.29%     
==========================================
  Files          89       89              
  Lines        4752     4799      +47     
  Branches      831      848      +17     
==========================================
+ Hits          814      836      +22     
- Misses       3428     3443      +15     
- Partials      510      520      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RayBB

Hey folks this is a great start!
I left a few small notes on the PR itself.

However, you also seem to have created a regression on the search results page.
Please fix that and the other comments and then I'll try to take a closer look.
Good luck!

http://localhost:8080/search?q=lewis&mode=everything

RayBB · 2024-12-17T19:16:59Z

static/css/layout/index.less

@@ -13,7 +13,7 @@ div.contentBody,
 div#contentBody {
  padding: 0 @contentBody-padding @contentBody-padding;
  img {
-    max-width: 100%;
+    // max-width: 100%;


If you want this to be removed please delete it completely instead of commenting it out.

RayBB · 2024-12-17T19:17:17Z

static/css/components/search-result-item.less

@@ -84,14 +84,12 @@
      filter: blur(2px);
    }
  }
-  .bookcover {
-    width: 100%;
+  .git {


Is this an intentional change? Probably a typo

RayBB · 2024-12-17T19:17:27Z

static/css/components/listLists.less

-        width: 32px;
-        max-width: 100%;
+        width: 22px;
+        // max-width: 100%;


Again, if you want this to be deleted please fully delete it.

RayBB · 2024-12-17T19:18:35Z

openlibrary/components/LibraryExplorer/components/OLCarousel.vue


-
 <style scoped>


Please try to keep the changeset small by avoiding whitespace changes like this one.

…ass, Removed unnecessary whitespace changes and Fixed regression issue on search results page Also, added a function to retrieve dimensions of a cover by its URL docs: add docstring to get_dimensions_cover function added newline at end of file in search-result-item.less format imports and clean up whitespace in book_providers.py

anna-ayn · 2024-12-18T00:45:20Z

Hi @RayBB

Thank you for your feedback! I've addressed the comments and made the necessary changes:

Removed Commented-Out Code: Fully deleted the commented-out max-width property in static/css/layout/index.less and in static/css/components/listLists.less
Fixed Typo in .bookcover Class: Removed changes made to static/css/components/search-result-item.less
Avoided Unnecessary Whitespace Changes: Removed unnecessary whitespace changes in openlibrary/components/LibraryExplorer/components/OLCarousel.vue.
Addressed Regression on Search Results Page: Fixed the regression issue on the search results page by modifying openlibrary/macros/SearchResultsWork.html.

RayBB

I've taken one more brief look and found two issues:

The cover in the search results has the wrong aspect ratio and as such is distorted.
The aspect ratio is also causing distortion in the "more by this author" section

Please fix the two above issues.

Also, please edit your original PR testing instructions to include a complete list of (localhost) URLs where we should see the changes.

As for the code itself, I didn't look closely as @cdrini is the lead and will likely have detailed feedback on that when he reviews it.

Overall, good work on this. It's certainly not an easy task to get this working in so many places.

…s related to covers in the search results

…e book.less Removed unnecessary changes in custom_carousel.html restored changes in custom_carousel.html

anna-ayn · 2024-12-20T17:44:03Z

Hi @RayBB,

I've resolved the distortion issue with the covers. It was related to the object-fit property setted as "cover", and I've made the necessary adjustments.

cdrini

Hi @anna-ayn , awesome work on this one! Unfortunately looking into it some more, we only really want to affect the book page, eg book_cover.html. Fetching the info for the cover width/height has a cost associated with it, so we don't want to be doing too frequently. On the book page, we only have the one main cover which causes the biggest layout shift, that's the one we want to address. The other pages, eg. search, carousels, won't be able to use the currently flow to get the width/height, because it would be too expensive to fetch that info for all those covers. There is a separate project for including this info in our solr search index which will make that possible, but for I'm afraid it's not.

Apologies for the run around here! We didn't realize the cost of this when creating the issue.

cdrini · 2025-02-05T17:29:58Z

openlibrary/plugins/openlibrary/js/lists/ShowcaseItem.js

This cover is very rarely displayed, and fetching the cover size before rendering it is not worth the complexity and performance costs, I believe, so we can roll back the changes to this piece (and where ShowcaseItem is called).

cdrini · 2025-02-05T17:33:27Z

openlibrary/templates/books/custom_carousel.html

This won't quite work, since it's hard-coding the aspect ratio to 180 / 360, but getting the image sizes for all the covers that will appear in a carousel is likely going to be expensive. I don't think that will be worth the trouble, so let's roll back this page here.

cdrini · 2025-02-05T17:34:49Z

openlibrary/templates/books/edition-sort.html

Ditto with this one, the editions table displays quite a few edition covers, and would be expensive to fetch the sizing info for all of them, so leaving this as it was is best. There is a separate project to include width/height in our search index solr; once that is complete, then we can use the information there to do this kind of work!

cdrini · 2025-02-05T17:37:40Z

openlibrary/macros/SearchResultsWork.html

Ditto here unfortunately, this will not be able to use/display the cover width/height until it is in solr.

cdrini · 2025-02-05T17:38:50Z

openlibrary/templates/covers/book_cover.html

 $if preload:
    $add_metatag(tag="link", **{'rel': 'preload', 'as': 'image', 'href': src, 'imagesrcset': srcset})

-<div class="coverMagic cover-animation">
+<div class="coverMagic cover-animation" style="min-height: ${calculated_height}px;">


It seems like we'll only need to specify the aspect ratio; e.g. style="aspect-ratio: $cover_width / $cover_height;" on the <img> element. @RayBB and I tested this, and it removed the layout shift! That should simplify this up quite a bit!

cdrini · 2025-02-05T17:39:28Z

openlibrary/templates/lists/home.html

Ditto here unfortunately, this will have to be rolled back to avoid a performance hit.

cdrini · 2025-02-05T17:44:12Z

openlibrary/book_providers.py

@@ -577,6 +581,24 @@ def get_cover_url(ed_or_solr: Edition | dict) -> str | None:
    return None


+def get_dimensions_cover(url: str) -> tuple[int, int] | None:


Since we have access to the database that contains all the cover info, instead of making a network request, it would be faster to instead make a database request.

I think calling the details method should fix this:

https://github.com/internetarchive/openlibrary/blob/8b2a94044f01b9388b96ff094559c8e4637b6d4f/openlibrary/coverstore/db.py

Add methods to get width and height in Image class

03fb650

anna-ayn marked this pull request as ready for review December 13, 2024 12:26

mekarpeles assigned RayBB Dec 16, 2024

anna-ayn and others added 8 commits December 16, 2024 21:36

Added width and height to img element for cover images in home templa…

932a6f8

…te of lists Remove docs for calculating cover image dimensions in home template

Add desiredHeight parameter to createActiveShowcaseItem function and …

ff001c3

…adjust image height to covers in already-lists

Remove max-width constraint for images in already-lists

4d9d777

Reduce image width in listLists component from 32px to 22px

f9820eb

fix trailing spaces Remove unnecessary blank lines in initMyBooksAffordances function

feat:fixes the height of the cover

5dce011

Estandarización de tamaños de las portadas de libros en carruseles

ed108a2

A new container is added and the size of the covers is standardized

cab1bb4

A new container is added and the size of the covers is standardized A new container is added and the size of the covers is standardized

Removal of the property 'width' from the .bookcover component in the …

b9956a4

…static/css/components/search-result-item.less file. applied black formatter

anna-ayn force-pushed the master branch from 9a94cd5 to b9956a4 Compare December 17, 2024 01:36

RayBB requested changes Dec 17, 2024

View reviewed changes

anna-ayn force-pushed the master branch from 7525785 to 4b1ebe4 Compare December 18, 2024 00:28

anna-ayn force-pushed the master branch from 9de026a to 8949f29 Compare December 18, 2024 00:35

RayBB requested changes Dec 20, 2024

View reviewed changes

anna-ayn added 3 commits December 20, 2024 09:35

changed object-fit property to "contain" in order to solve distorsion…

94b8748

…s related to covers in the search results

Add aspect ratio handling for book cover images in edition table

54d132c

Implement aspect ratio handling for book covers in carousel and updat…

8b388aa

…e book.less Removed unnecessary changes in custom_carousel.html restored changes in custom_carousel.html

anna-ayn force-pushed the master branch from 180fbb4 to 8b388aa Compare December 20, 2024 17:32

anna-ayn requested a review from RayBB December 20, 2024 17:54

RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Dec 22, 2024

mekarpeles assigned cdrini Dec 23, 2024

cdrini requested changes Feb 5, 2025

View reviewed changes

cdrini added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. labels Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using book cover sizes to prevent layout shifting #10145

Using book cover sizes to prevent layout shifting #10145

anna-ayn commented Dec 13, 2024 •

edited

Loading

anna-ayn commented Dec 14, 2024

mekarpeles commented Dec 16, 2024 •

edited

Loading

anna-ayn commented Dec 17, 2024 •

edited

Loading

codecov-commenter commented Dec 17, 2024 •

edited

Loading

RayBB left a comment

RayBB Dec 17, 2024

RayBB Dec 17, 2024

RayBB Dec 17, 2024

RayBB Dec 17, 2024

anna-ayn commented Dec 18, 2024

RayBB left a comment

anna-ayn commented Dec 20, 2024

cdrini left a comment

cdrini Feb 5, 2025

cdrini Feb 5, 2025

cdrini Feb 5, 2025

cdrini Feb 5, 2025

cdrini Feb 5, 2025

cdrini Feb 5, 2025

cdrini Feb 5, 2025

		@@ -577,6 +581,24 @@ def get_cover_url(ed_or_solr: Edition \| dict) -> str \| None:
		return None


		def get_dimensions_cover(url: str) -> tuple[int, int] \| None:

Using book cover sizes to prevent layout shifting #10145

Are you sure you want to change the base?

Using book cover sizes to prevent layout shifting #10145

Conversation

anna-ayn commented Dec 13, 2024 • edited Loading

Technical

Improvements to cover image handling:

Updates to CSS styles:

Template modifications:

Testing

Screenshot

Stakeholders

anna-ayn commented Dec 14, 2024

mekarpeles commented Dec 16, 2024 • edited Loading

anna-ayn commented Dec 17, 2024 • edited Loading

codecov-commenter commented Dec 17, 2024 • edited Loading

Codecov Report

RayBB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anna-ayn commented Dec 18, 2024

RayBB left a comment

Choose a reason for hiding this comment

anna-ayn commented Dec 20, 2024

cdrini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anna-ayn commented Dec 13, 2024 •

edited

Loading

mekarpeles commented Dec 16, 2024 •

edited

Loading

anna-ayn commented Dec 17, 2024 •

edited

Loading

codecov-commenter commented Dec 17, 2024 •

edited

Loading