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

Implement [YouTube] badge #5132

Merged
merged 8 commits into from
Jun 10, 2020
Merged

Implement [YouTube] badge #5132

merged 8 commits into from
Jun 10, 2020

Conversation

PyvesB
Copy link
Member

@PyvesB PyvesB commented May 25, 2020

Fixes #5101.

Four different statistics can be displayed in the badge: view, like, dislike and comment counts.

@PyvesB PyvesB added the service-badge New or updated service badge label May 25, 2020
@shields-cd shields-cd temporarily deployed to shields-staging-pr-5132 May 25, 2020 17:30 Inactive
@shields-ci
Copy link

shields-ci commented May 25, 2020

Warnings
⚠️ This PR modified the server but none of its tests.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @PyvesB!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 6a90fde

@shields-cd shields-cd temporarily deployed to shields-staging-pr-5132 May 25, 2020 17:39 Inactive
@PyvesB PyvesB temporarily deployed to shields-staging-pr-5132 May 25, 2020 17:44 Inactive
@PyvesB
Copy link
Member Author

PyvesB commented May 25, 2020

Example badge from the review app:
https://shields-staging-pr-5132.herokuapp.com/youtube/views/abBdk8bSPKU?logo=youtube

@MohamedAliRashad feel free to have a play around :)

@MohamedAliRashad
Copy link

@PyvesB
Looks Great, it would be nicer though if the youtube logo was black and red but anyway it's beautiful and the effort is appreciated.

One small detail i want to point also, make sure the view counts adapt to bigger numbers with K and M abbreviations for thousands and millions.

static get route() {
return {
base: 'youtube',
pattern: ':statistic(view|like|dislike|comment)/:videoId',
Copy link
Member

Choose a reason for hiding this comment

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

Could I suggest instead of a dislikes badge, we do a query param variant on the likes badge, that shows both likes and dislikes?

votes | 2.5k up, 100 down

It's a little like the way we do failing tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, will implement sometime this week!

@PyvesB
Copy link
Member Author

PyvesB commented May 25, 2020

@PyvesB
Looks Great, it would be nicer though if the youtube logo was black and red but anyway it's beautiful and the effort is appreciated.

Thanks for the feedback! The icons Shields uses come from simple-icons, though you can override those by specifying the icon data as Base 64 for example. 😉

One small detail i want to point also, make sure the view counts adapt to bigger numbers with K and M abbreviations for thousands and millions.

Yep, that's taken care of with our standard metric helper function.

@PyvesB
Copy link
Member Author

PyvesB commented May 30, 2020

@MohamedAliRashad
Copy link

@PyvesB
It will be super cool if the right half part is configurable like showing the number of likes and subscribers in a channel or the view count with nothing else next to it.

@PyvesB
Copy link
Member Author

PyvesB commented May 30, 2020

@PyvesB
It will be super cool if the right half part is configurable like showing the number of likes and subscribers in a channel or the view count with nothing else next to it.

It is:
https://shields-staging-pr-5132.herokuapp.com/youtube/likes/pU9Q6oiQNd0?logo=youtube
https://shields-staging-pr-5132.herokuapp.com/youtube/views/pU9Q6oiQNd0?logo=youtube

@MohamedAliRashad
Copy link

@PyvesB
I meant the number of likes next to it the number of views (not seperate), like the likes and dislikes logo.

@PyvesB
Copy link
Member Author

PyvesB commented May 31, 2020

@PyvesB
I meant the number of likes next to it the number of views (not seperate), like the likes and dislikes logo.

We tend to be quite careful when it comes to displaying several pieces of information in a single badge. Here I think it's fine to have likes and dislikes at the same time given that they're closely related concepts and the thumbs up/down UTF-8 characters clearly convey the information at a glance. However, I would not be inclined to allow users to add view count, comments channel subscribers or other loosely related information in the same badge, things would become bloated and less legible.

}

static get defaultBadgeData() {
return { label: 'youtube', color: 'red' }
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts on making this a "social" styled badge, like the Twitter one?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable to me, especially since we're embedding the link (which AFAIK is something we only do on social badges)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

label: 'votes',
message: `${metric(statistics.likeCount)} 👍 ${metric(
statistics.dislikeCount
)} 👎`,
Copy link
Member

Choose a reason for hiding this comment

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

Not as part of this PR, but perhaps as a future enhancement (should there be any interest) we could allow users to configure the up/down values like we do for the various test result badges

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, this can be added in the future if users ask for it!

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

I haven't been tracking this one too closely, but the code LGTM. Happy to approve if needed (holding off for the moment in case you want to go ahead with changing the category/style)

@PyvesB PyvesB merged commit 0fd557d into badges:master Jun 10, 2020
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YouTube Badge
6 participants