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

Reviewer Badges :) #1389

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link
Contributor

@sneakers-the-rat sneakers-the-rat commented Nov 2, 2024

Fix: #1385

here she is:

review_badge.mp4

and on mobile:

review_badge_mobile.mp4

OK this was more complicated than i thought because it turns out you need to like calculate the widths of all the characters for a given font to make an SVG badge, but i did it because i have wanted to have something like this in my own websites without having to use shields.io or w/e. Elsewhere the SVGs are just hardcoded, but I thought it would be nice to generate them for any key and color because we might want to use them for other things :)

I basically transcribed and simplified shields.io's code, it's mostly self explanatory except don't ask me what's going on with those scaling parameters. I grabbed an array of codepoint widths and made it a constant, use that to calculate the width, and the rest is in the controller.

I tried to reuse as much as i could from the rest of the site, but it looks like we're not using sass variables so i just hunted out values that seemed like they were the right ones to use.

The badges will be at joss.theoj.org/badges/reviewed_by/:username. I imagine re-rendering them from scratch every time is pointlessly expensive, so plz lmk how caching works here because they should only rarely change, but it's hard to detect the review count being stale because we don't have a Reviewer model.

I am holding off on tests for now until i get signoff on the basic approach, and then i'll write em!

@arfon
Copy link
Member

arfon commented Nov 5, 2024

Hey! Thanks for getting going on this @sneakers-the-rat ⚡. This is definitely the sort of thing I'm thinking about. One thought, do you think as part of this we might want to consider consolidating the other SVG rendering (e.g.,

def status
if params[:doi] && valid_doi?
@paper = Paper.find_by_doi(params[:doi])
else
@paper = Paper.find_by_sha(params[:id])
end
# TODO: Remove these SVGs from the controller
if @paper
svg = @paper.status_badge
else
prefix = setting(:abbreviation)
svg = "<svg xmlns='http://www.w3.org/2000/svg' width='102' height='20'><linearGradient id='b' x2='0' y2='100%'><stop offset='0' stop-color='#bbb' stop-opacity='.1'/><stop offset='1' stop-opacity='.1'/></linearGradient><mask id='a'><rect width='102' height='20' rx='3' fill='#fff'/></mask><g mask='url(#a)'><path fill='#555' d='M0 0h40v20H0z'/><path fill='#9f9f9f' d='M40 0h62v20H40z'/><path fill='url(#b)' d='M0 0h102v20H0z'/></g><g fill='#fff' text-anchor='middle' font-family='DejaVu Sans,Verdana,Geneva,sans-serif' font-size='11'><text x='20' y='15' fill='#010101' fill-opacity='.3'>#{prefix}</text><text x='20' y='14'>#{prefix}</text><text x='70' y='15' fill='#010101' fill-opacity='.3'>Unknown</text><text x='70' y='14'>Unknown</text></g></svg>"
end
) into a common controller?

@sneakers-the-rat
Copy link
Contributor Author

I think it would make sense to do that, but didnt want to change it all until I got a nod to go ahead :)

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Nov 13, 2024

reviewing my open PRs and thought i'd ping on this?

The only outstanding question i think is caching, and from what I can tell from the docs, rails should automatically cache rendering of the template for a given set of parameters. since the parameters given to the template are all generic across reviewer identity (i.e. they just have the text of the badge and the numbers) that should kick in. Ideally we would also cache the badge per reviewer so we don't need to do a db query at all, but doing that would require us to add reviewers to the schema so we can do cache invalidation when a reviewer does another review, and that should probably be a different PR.

Another approach would be to make the badge endpoint totally generic, someting like example.com/api/badges?key=JOSS%20Reviews&value=20&value_color=blue so that every badge could be cached on its own, and then the badges/reviewed_by/:username endpoint could just forward to the relevant badge page, so rather than rendering a badge per user we are rendering a badge per parameterization.

I also think swapping the existing badges to this controller should probably be another PR.

Anything blocking this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub badges for review (and papers edited) counts
2 participants