-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add activity feed to My Books page #11391
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are accessibility issues in these changes.
|
|
||
| $def render_images(): | ||
| $for url in image_urls: | ||
| <img src="$url" loading="lazy" width="80"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This image is missing a text alternative. This is a problem for people using screen readers.
| SELECT EXISTS( | ||
| SELECT 1 | ||
| FROM follows | ||
| WHERE subscriber=$subscriber | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick reference to EXISTS documentation, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors list showcase cards to use a reusable multi-image card component and adds an activity feed feature to the mybooks page. The changes extract common styling and markup into a shared component to reduce code duplication.
- Introduces a new
multi-image-cardcomponent with reusable styles and template - Adds an activity feed feature that displays recent bookshelf events
- Refactors existing list showcase and list follow components to use the new shared component
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| static/css/components/multi-image-card.less | New component with shared styles for multi-image cards |
| static/css/components/activity-feed.less | Styles for activity feed cards using the multi-image-card base |
| static/css/components/list-showcase.less | Refactored to use multi-image-card component, removing duplicate styles |
| static/css/components/list-follow.less | Refactored to use BEM naming and removed inline styles |
| openlibrary/templates/cards/multi_image_card.html | New reusable template for multi-image cards |
| openlibrary/templates/lists/showcase.html | Updated to use the new multi_image_card template |
| openlibrary/templates/lists/list_follow.html | Refactored to use the new card template |
| openlibrary/templates/account/activity_feed.html | New template for rendering activity feeds |
| openlibrary/templates/account/mybooks.html | Updated to include activity feed component |
| openlibrary/plugins/upstream/mybooks.py | Added ActivityFeed class and integration |
| openlibrary/core/follows.py | Added is_following method to check if user follows others |
| static/css/page-book.less | Added import for multi-image-card component |
| static/css/components/mybooks.less | Added import for activity-feed component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <p>$_("Here's the latest activity around the library. Follow readers to personalize your feed.")</p> | ||
| $elif not feed: | ||
| <p>$_("None of the people that you follow have logged books. When they do, you'll see it here.")</p> | ||
| <div class="list-showcase"> |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opening <div class=\"list-showcase\"> tag at line 48 is missing its corresponding closing tag. This will result in invalid HTML structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reject your spurious claim. The closing tag is on line 61...
| margin-top: -10px; | ||
| position: relative; | ||
| z-index: @z-index-level-3; | ||
| border: 2px solid @white; |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The img nested within .list-follow-card__user is missing the flex-shrink: 0; property that was present in the original code. This could cause layout issues when the container width is constrained.
| border: 2px solid @white; | |
| border: 2px solid @white; | |
| flex-shrink: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| border-radius: 4px; | ||
| box-shadow: 2px 2px 4px fade(@black, 15%); | ||
| } | ||
|
|
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon in CSS property declaration. Should be display: flex; for consistency with project style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh?
But seriously, I wonder if there is a race condition between the pre-commit linting and the copilot review?
| transform: translate(0, 20px); | ||
| } | ||
|
|
||
| img:nth-child(2) { | ||
| z-index: @middle-cover-z-index; | ||
| transform: translate(-8.5px, 20px); | ||
| } | ||
|
|
||
| img:nth-child(3) { | ||
| z-index: @bottom-cover-z-index; | ||
| transform: translate(-17px, 20px); |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded magic numbers (20px, 8.5px, 17px) for positioning. Consider using a variable like @cover-padding which is already defined at line 3 for consistency and maintainability.
| transform: translate(0, 20px); | |
| } | |
| img:nth-child(2) { | |
| z-index: @middle-cover-z-index; | |
| transform: translate(-8.5px, 20px); | |
| } | |
| img:nth-child(3) { | |
| z-index: @bottom-cover-z-index; | |
| transform: translate(-17px, 20px); | |
| transform: translate(0, @cover-padding); | |
| } | |
| img:nth-child(2) { | |
| z-index: @middle-cover-z-index; | |
| transform: translate(-0.425 * @cover-padding, @cover-padding); | |
| } | |
| img:nth-child(3) { | |
| z-index: @bottom-cover-z-index; | |
| transform: translate(-0.85 * @cover-padding, @cover-padding); |
| transform: translate(0, 20px); | ||
| } | ||
|
|
||
| img:nth-child(2) { | ||
| z-index: @middle-cover-z-index; | ||
| transform: translate(-8.5px, 20px); | ||
| } | ||
|
|
||
| img:nth-child(3) { | ||
| z-index: @bottom-cover-z-index; | ||
| transform: translate(-17px, 20px); |
Copilot
AI
Oct 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded magic numbers (20px, 8.5px, 17px) for positioning. Consider using a variable like @cover-padding which is already defined at line 3 for consistency and maintainability.
| transform: translate(0, 20px); | |
| } | |
| img:nth-child(2) { | |
| z-index: @middle-cover-z-index; | |
| transform: translate(-8.5px, 20px); | |
| } | |
| img:nth-child(3) { | |
| z-index: @bottom-cover-z-index; | |
| transform: translate(-17px, 20px); | |
| transform: translate(0, @cover-padding); | |
| } | |
| img:nth-child(2) { | |
| z-index: @middle-cover-z-index; | |
| transform: translate(-@cover-overlap, @cover-padding); | |
| } | |
| img:nth-child(3) { | |
| z-index: @bottom-cover-z-index; | |
| transform: translate(-2 * @cover-overlap, @cover-padding); |
| @card-width: 215px; | ||
| @cover-width: 64px; | ||
| @cover-padding: 20px; | ||
| @top-cover-z-index: 3; | ||
| @middle-cover-z-index: 2; | ||
| @bottom-cover-z-index: 1; | ||
| @cover-overlap: calc( | ||
| (3 * @cover-width - (@card-width - 2 * @cover-padding)) / 2 | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe for these to be declared at the top of the file? Maybe they should be nested to avoid overwriting other similarly named Less variables.
02e9d5b to
abe95d5
Compare
b942fd5 to
1ea032f
Compare
22a1e70 to
64d7a86
Compare
ab055c0 to
6b360d8
Compare
b539c6d to
c9b6949
Compare
for more information, see https://pre-commit.ci
|
|
||
| @classmethod | ||
| def get_cached_trending_feed(cls): | ||
| five_minutes = 5 * dateutil.MINUTE_SECS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In testing, the "My Books" page took a bit too long to initially load. On refresh, the page loaded very fast.
It may be worthwhile to increase the cache time for the trending feed to something much larger than 5 minutes. Basically, we want to show patrons who are not following others that books are being cataloged -- it need not be the most recent changes. Maybe an hour?
| $for i in feed: | ||
| $ shelf_name = shelf_id_to_name[i.get('bookshelf_id')] | ||
| $ occurred_on = datestr(i.get("created")) | ||
| $ cover = get_cover_url(i) or "/images/icons/avatar_book-sm.png" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is producing incorrect covers. Is there a way to fetch a cover given a work ID (without unmarshalling the data into a model)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be easier to spot if we were rendering these activity feed cards with a work title.
| $def anchored_header(): | ||
| <a href="$href" class="multi-img-card__header"> | ||
| $:header_html | ||
| </a> | ||
|
|
||
| $def header(): | ||
| <div class="multi-img-card__header"> | ||
| $:header_html | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make much sense, in retrospect. Developers will be passing in an HTML string for the header -- they should just wrap it in an anchor if they want it to be a link.
|
Also related to #10241 |



Closes #10242
Updates the application such that:
/cards/multi_image_card.html).Technical
A new
PubSubmethod has been created for the purpose of determining if a patron follows any other person on the site.The
multi_image_cardcardThe new multi-image card is composed of three sections:
Developers are expected to pass a valid HTML string for the card's header and footer, and a list of URLs for images that will be rendered in the card.
The new
multi_image_cardhas been designed to either be nested in an anchor element (like the "My Lists" cards on the My Books page), or to contain multiple anchor elements within the card itself (like the cards in the "Lists" section of book pages.If a URL is passed to the template (via the
hrefparameter), both the header and the image container will be wrapped by anchor elements that link to the given URL. If the entire card is rendered within an anchor element,hrefshould not be set as this will result in nested anchor tags.Testing
Book Page:
My Books Page -- My Lists section
My Books Page -- My Feed section
If not following others, expect:
/trendingfeedIf following only people that do not log books, expect:
If following people who do log books, expect:
For all cases, expect:
Screenshot
"My Feed" for patrons who follow active patrons.
"My Feed" for patrons who do not follow anybody.
Stakeholders