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

add a grid mode to the carousel macro #8369

Merged

Conversation

davidscotson
Copy link
Collaborator

Closes #7778 (or at least provides an alternative way for librarians to achieve their goals)

feature: add grid_mode=True to the Carousel macros to get a grid view instead.

Technical

  1. this basically ignores the load_more code path. So you need to load your full grid in one go.
  2. I've not tested in IE11, but it should be relatively simple to tweak the grid stuff to work if there's issues. Might just need display: -ms-grid; but not sure if prefixing is already automated in webpack.

Testing

Find/create a page with a Carousel macro and add grid_mode=True e.g.

{{ListCarousel("/people/openlibrary/lists/OL1L", limit=20, has_fulltext_only=False, url='https://openlibrary.org/people/openlibrary/lists/OL1L', grid_mode=True)}}

Screenshot

Screenshot 2023-10-04 at 20 58 15

Stakeholders

@cdrini, @digitals82

@mekarpeles mekarpeles self-assigned this Oct 5, 2023
@digitals82
Copy link

Awesome! Thanks for this @davidscotson .

@cdrini cdrini added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Oct 5, 2023
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Works beautifully!!
image

Tested IE and iOS 9 and flex-wrap fixed the non-grid support. Used https://testing.openlibrary.org/collections/tv-people-books

static/css/components/carousel.less Outdated Show resolved Hide resolved
@mekarpeles mekarpeles assigned cdrini and unassigned mekarpeles Oct 5, 2023
@mekarpeles
Copy link
Member

I think @cdrini wants to take this one. A few points to consider:

  • Hopefully we have some bound (e.g. the 20 specified) that prevents the page from being unreasonably long
  • In the cases where there is a limit, we may want to have some sort of load_more

@digitals82
Copy link

@mekarpeles and @cdrini There are definitely series that have more than 20 books in them, so a load more or the ability to adjust how many are displayed would be great.

Thanks

@davidscotson
Copy link
Collaborator Author

Regarding the load_more stuff, is there really any notable performance difference if you load 40 or even 60 or 80 items from a list? My gut reaction is there isn't likely to be. So if there is a default for when load_more kicks in I'd assume it should be higher than the carousel default (where multiple carousels per page are common and only 6 are viewable until the user interacts).

I'm assuming it's the equivalent of a single DB call on the server side, though that is just an assumption. The images are already marked as loading="lazy" which will account for the bulk of the network connections and file transfers.

I'm assuming the main use of this is for individual pages with long lists, but my gut is feeling like a default limit of 100 that loads in one go would cover almost everything and once you go above that you want a whole different interface anyway.

I'll see if I can come up with any tests to prove or disprove my intuition on this.

@davidscotson
Copy link
Collaborator Author

davidscotson commented Oct 6, 2023

I tried testing here https://testing.openlibrary.org/collections/100-best-mystery-and-thriller-books-of-all-time but it seems to have got my first edit with a limit of 20 stuck in a cache or something, as it's ignoring my later edits to try different amounts or any other text change.

edit: worked around it by specifying a version in the url:

https://testing.openlibrary.org/collections/100-best-mystery-and-thriller-books-of-all-time?v=13

@davidscotson davidscotson force-pushed the feature/7778_grid_display branch from ec7726f to 67e9f46 Compare October 6, 2023 12:04
@cdrini
Copy link
Collaborator

cdrini commented Oct 6, 2023

Sweet! This all looks good :) Setting limit=N will let you display more books @digitals82 .

@cdrini cdrini force-pushed the feature/7778_grid_display branch from 32fb843 to 79f7ad8 Compare October 6, 2023 14:10
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

On testing works like a charm!

@codecov-commenter
Copy link

Codecov Report

Merging #8369 (79f7ad8) into master (35d6b72) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8369   +/-   ##
=======================================
  Coverage   16.66%   16.66%           
=======================================
  Files          83       83           
  Lines        4422     4422           
  Branches      757      757           
=======================================
  Hits          737      737           
  Misses       3202     3202           
  Partials      483      483           

@cdrini cdrini merged commit 0857026 into internetarchive:master Oct 6, 2023
3 checks passed
@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carousels bug with window resizing.
6 participants