-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 Star Ratings & Want To Read counts to Search Result Cards #9790
Add Star Ratings & Want To Read counts to Search Result Cards #9790
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9790 +/- ##
==========================================
+ Coverage 16.06% 16.42% +0.36%
==========================================
Files 90 92 +2
Lines 4769 4907 +138
Branches 832 856 +24
==========================================
+ Hits 766 806 +40
- Misses 3480 3565 +85
- Partials 523 536 +13 ☔ View full report in Codecov by Sentry. |
fcbbff6
to
8e3602c
Compare
$if doc.get('languages'): | ||
<span class="languages"> | ||
$:ungettext('in <a class="hoverlink" title="%(langs)s">%(count)d language</a>', 'in <a class="hoverlink" title="%(langs)s">%(count)d languages</a>', len(doc.languages), count=len(doc.languages), langs=commify_list([get_language_name('/languages/' + lang) for lang in doc.languages])) |
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.
Full Text Searching for the fun explosions in the sky album name "all of a sudden i miss everyone" results in a bug
https://testing.openlibrary.org/search/inside?q=all+of+a+sudden+i+miss+everyone&debug=true
Bonus 🙂 |
543a0a7
to
3b66336
Compare
858010b
to
51edb5c
Compare
Completed the following updates:
I have questions on these ones:
|
Re #555, 🤣 I'm so sorry, that was meant to be an html code |
@@ -0,0 +1,20 @@ | |||
$def with(ratings_count, ratings_average, want_to_read_count, page_type) |
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.
want_to_read_count
probably shouldn't be in the Star Ratings General
Things are looking pretty good! Just 3 more things that need to be tweaked:
|
9cf101d
to
a2d890c
Compare
a2d890c
to
173c1b3
Compare
@mekarpeles - thank you for follow-up review.
|
@mekarpeles I think I need to input some mock data because I am not able to reproduce the styling you are seeing. I'm not sure how to replicate half stars and 4 digit numbers in my test environment. On my end, the following adjustments have been made:
|
206893e
to
bc17e5d
Compare
2 bugshttps://testing.openlibrary.org/books/OL17236630M/The_Frodo_franchise?debug=true
https://testing.openlibrary.org/search/inside?q=lord+of+the+rings I believe that because Also, in the case of Fulltext Search Inside, I believe we're getting a list of infogami editions back (instead of solr ** We should be able to modify it by adding a type check... e.g.
|
May be a bug with half-stars. In some cases, we may be rounding up the stars but the half-stars class is still being applied? https://testing.openlibrary.org/search?q=lord+of+the+rings&mode=ebooks&has_fulltext=true |
7ce1e36
to
8b4e76d
Compare
@merwhite11 thank you for going through these! 🎉 |
7d5ba0b
to
ae5e52b
Compare
for more information, see https://pre-commit.ci
Some small bug fixes
|
$ want_to_read_count = "{:,}".format(stats_by_bookshelf.get('want-to-read', None)) | ||
$ currently_reading_count = "{:,}".format(stats_by_bookshelf.get('currently-reading', None)) | ||
$ already_read_count = "{:,}".format(stats_by_bookshelf.get('already-read', None)) | ||
$ review_count_class = 'readers-stats__review-count--none' if ratings_count == None else '' | ||
$ id = '--mobile' if mobile else '' | ||
|
||
<ul class="readers-stats $review_count_class" itemprop="aggregateRating" itemscope itemtype="https://schema.org/AggregateRating"> |
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.
<ul class="readers-stats $review_count_class" itemprop="aggregateRating" itemscope itemtype="https://schema.org/AggregateRating"> | |
<ul class="readers-stats $review_count_class" itemprop="aggregateRating" itemscope itemtype="https://schema.org/AggregateRating"> |
One bug we may want to look into: https://testing.openlibrary.org/works/OL17860744W/A_Court_of_Mist_and_Fury?debug=true This book seems to have exactly 4.0 rating avg but shows a half star 🤔 This book shows 4.2 stars (which should show half?) but shows no half star The good news is, in both cases, this also seems to be the behavior on production? |
Closes #9463
Pulls ratings_count, ratings_average and want_to_read_count fields from Solr.
Creates a new macro that accesses these fields and displays ratings_count in star graphic and ratings_count and want_to_read_count in Search Results cards.
Technical
Testing
When testing in my local environment, I manually updated the star ratings on the books page to trigger the StarRatingsByline macro to display.
Screenshot
Questions
I created a conditional that requires a minimum of 1 ratings_count and 1 want_to_read_count in order for the StarRatingsByline to display. Should I split up the conditional so that star ratings will display, even if there is no want_to_read_count ?
Stakeholders
@mekarpeles