www.fgks.org   »   [go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Add Half Star Rating #4401

Merged
merged 11 commits into from
Apr 26, 2021
Merged

Add Half Star Rating #4401

merged 11 commits into from
Apr 26, 2021

Conversation

Yashs911
Copy link
Contributor
@Yashs911 Yashs911 commented Jan 11, 2021 •
edited

Closes #4371
Percentage based star rating

Screeenshots:

Hard coded rating value
image

Stakeholders

@jdlrobson

@Yashs911 Yashs911 mentioned this pull request Jan 11, 2021
Closed
@jdlrobson jdlrobson added the Priority: 4 An issue, but should be worked on when no other pressing work can be done. [managed] label Jan 11, 2021
@jdlrobson jdlrobson self-assigned this Jan 12, 2021
@jdlrobson jdlrobson self-requested a review January 12, 2021 04:19
jdlrobson
jdlrobson previously requested changes Jan 12, 2021
View reviewed changes
Copy link
Collaborator
@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

Per issue, I don't think we should resort to additional svgs here. Partial stars are great, but we should use CSS.

Not a high priority issue right now. Happy to help find you tasks with a higher priority - just let me know in Slack :)

@Yashs911
Copy link
Contributor Author
Yashs911 commented Jan 30, 2021 •
edited

Hi @jdlrobson I have implemented pure CSS (percentage-based) star ratings 😃
Can you please review this PR again?
image

@Yashs911 Yashs911 changed the title Half Star Rating Add Percentage based Star Ratings Jan 30, 2021
@Yashs911 Yashs911 mentioned this pull request Feb 15, 2021
Merged
@cclauss cclauss added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Mar 16, 2021
@cclauss
Copy link
Collaborator
cclauss commented Mar 16, 2021

Labeling as blocked because of failing tests.

@Yashs911
Copy link
Contributor Author
Yashs911 commented Mar 22, 2021 •
edited

@jdlrobson the JS tests ate failing due to: FAIL static/build/page-edit.css: 20.01KB > maxSize 20KB (gzip)
Should I increase the size?

@jdlrobson
Copy link
Collaborator

Sure. 0.1kb is pretty negligible. Please bump to 21kb.

@Yashs911
Copy link
Contributor Author

@jdlrobson Increased the bundlesize.

@cclauss cclauss removed the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Mar 24, 2021
@Yashs911
Copy link
Contributor Author
Yashs911 commented Apr 4, 2021

@jdlrobson I have updated the PR to use only CSS to display half stars

jdlrobson
jdlrobson requested changes Apr 23, 2021
View reviewed changes
Copy link
Collaborator
@jdlrobson jdlrobson left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this one. Functionality-wise I think this is a good compromise. It's simple and useful to the user.

Slight refactor request to the code to make it easier to work with in future.

openlibrary/macros/StarRatingsStats.html Outdated
@@ -6,7 +6,11 @@
<ul class="readers-stats" itemprop="aggregateRating" itemscope itemtype="https://schema.org/AggregateRating">
<li class="avg-ratings">
$if rating_stats.get('avg_rating'):
$:('<span class="readers-stats__star">★</span>' * int(rating_stats.get('avg_rating'))) <span itemprop="ratingValue">$(rating_stats['avg_rating'])</span>
$ stats_decimal = (float(rating_stats.get('avg_rating')))-(int(rating_stats.get('avg_rating')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than repeating rating_stats.get('avg_rating')
I'd suggest reading it and caching it once.

e.g.

$ avg = rating_stats.get('avg_rating')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the changes as requested

openlibrary/macros/StarRatingsStats.html Outdated
$:('<span class="readers-stats__star">★</span>' * int(rating_stats.get('avg_rating')))
$if (stats_decimal >= 0.5) and (stats_decimal < 1):
$:('<span class="readers-stats__star--half">★</span>')
<span itemprop="ratingValue">$(rating_stats['avg_rating'])</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why rating_stats['avg_rating'] here and not rating_stats.get('avg_rating') (or using a cached variable as above) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used $avg

openlibrary/macros/StarRatingsStats.html
$ stats_decimal = (float(rating_stats.get('avg_rating')))-(int(rating_stats.get('avg_rating')))
$:('<span class="readers-stats__star">★</span>' * int(rating_stats.get('avg_rating')))
$if (stats_decimal >= 0.5) and (stats_decimal < 1):
$:('<span class="readers-stats__star--half">★</span>')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe in future we should also output the empty stars (but out of scope for now :))

@Yashs911
Copy link
Contributor Author

Failing tests:
FAIL static/build/page-book.css: 9.64KB > maxSize 9.6KB (gzip)
FAIL static/build/page-form.css: 20.01KB > maxSize 20KB (gzip)

@jdlrobson jdlrobson merged commit 62b8d1a into internetarchive:master Apr 26, 2021
@Yashs911 Yashs911 changed the title Add Percentage based Star Ratings Add Half Star Rating Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdlrobson jdlrobson jdlrobson approved these changes

Assignees

@jdlrobson jdlrobson

Labels
Priority: 4 An issue, but should be worked on when no other pressing work can be done. [managed]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rating Stats round off stars to lowest integer
3 participants
@Yashs911 @cclauss @jdlrobson