-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[amo chrome-web-store wordpress] Enhanced star display #1179
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.
Love this!
Agreed that it makes a decent tradeoff between attractiveness and usefulness.
if (decimal >= 0.875) { stars += '★'; } | ||
else if (decimal >= 0.625) { stars += '¾'; } | ||
else if (decimal >= 0.375) { stars += '½'; } | ||
else if (decimal >= 0.125) { stars += '¼'; } | ||
while (stars.length < 5) { stars += '☆'; } | ||
return stars; | ||
} |
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.
Could you add unit tests of this function, like the ones for maybePluralize
in https://github.com/badges/shields/blob/master/lib/text-formatters.spec.js? The tests make it easy to verify it's doing what's expected and intended, and prevent regressions in case someone needs to change this in the future.
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 had missed out on that file. Will fix it.
@@ -11,7 +11,7 @@ const isVPlusTripleDottedVersion = withRegex(/^v[0-9]+.[0-9]+.[0-9]+$/); | |||
|
|||
const isVPlusDottedVersionAtLeastOne = withRegex(/^v\d+(\.\d+)?(\.\d+)?$/); | |||
|
|||
const isStarRating = withRegex(/^[\u2605\u2606]{5}$/); | |||
const isStarRating = withRegex(/^(?=.{5}$)(\u2605{0,5}[\u00BC\u00BD\u00BE]?\u2606{0,5})$/); |
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.
Nice!
Ace. Thanks! |
Ran all the service tests locally on this branch plus #1118, and got two unrelated timeouts:
|
This is another follow up pull request, related to some discussions in #1177.
Brief outline of the problem, for people who haven't read the previous comments. The
starRating
method was previously displaying stars floored to the nearest integer. If 4.99 was passed in as an argument, it would display ★★★★☆. Even if the value was rounded before passing it as an argument (this was done for the Chrome badges), values such as 4.49 would be brought down to ★★★★☆, which seems rough.This pull request attempts to solve the problem by making use of three additional UTF-8 characters. Badges will now display ratings such as the following:
Again, lacking UTF-8 characters such as half stars is not ideal, but this is a trade-off between having a badge that looks perfect and having a badge that actually conveys meaningful information. 🌟
Cheers,
Pyves