-
Notifications
You must be signed in to change notification settings - Fork 565
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
Implemented option to show like/dislike percentage instead of full like/dislike count #560
Conversation
My bad, force-pushed to fix 2 things I missed because didn't notice this: "Add more commits by pushing to the main branch on ErykDarnowski/return-youtube-dislike." |
Extensions/combined/popup.html
Outdated
<div class="custom-select"> | ||
<label for="tooltip_percentage_mode" data-hover="Use custom percentage display on hover.">Percent mode:</label> | ||
<select name="tooltip_percentage_mode" id="tooltip_percentage_mode"> | ||
<option value="dash_like" id="tooltip_percentage_mode_dash_like">Dash like</option> |
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.
Will it be easier for you and users if the <option>
innerHTML is indeed the example: 190 / 10 - 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.
Right, I created an example element the same way theme colors have one, but you've got a point. Let me try this out.
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.
Done. It honestly looks and feels much better, thank you for the suggestion!
Just want to point out this feature is already implemented in this extension: Thumbnail Rating Bar for YouTube™ I've been using it since the same time I got RYD. Anyway, good work. |
BTW could you take a look at #540? Your feature would be also great for userscript. |
Yeah, I saw your issue so I'm planning to do that ;). Also I think it's still nice to not have to install a couple extension for features related to each other. |
I'm not so worried. I use that extension mainly for batch examining. It happens to put a percentage in the ration bar. |
@@ -29,6 +29,9 @@ let previousState = 3; //1=LIKED, 2=DISLIKED, 3=NEUTRAL | |||
let likesvalue = 0; | |||
let dislikesvalue = 0; | |||
|
|||
// Temporary user config variable until better solution is found: | |||
var tooltipPercentageDisplayOption = "classic"; // classic ; dash_like ; dash_dislike ; both ; only_like ; only_dislike |
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.
For future consideration, you may take a look at #540. We can mirror the data structure in the extension.
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.
Sure, I'll probably work on this today 😉
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'm wondering if it would be a good idea to add a boolean value for switching the percent display on/off like I've done it in the extension popup window. Although it could totally be done in just one var like it is at the moment.
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.
What about adding a list option 'none'?
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.
There is an option called "classic"
already which works like "none"
would :). Although I guess "none"
would make more sense. I used the same default value as I think colorTheme
did.
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.
The color thing has a separate switch so you can control two things differently, thumbs and bar. If i made the "none" option in the color theme then i can only turn everything on and off together.
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.
Yeah, yeah I get that. I was just wondering if it should replicate the extension popup, but there isn't really a good reason for it.
"only_dislike": `${dislikePercentage}%` | ||
}; | ||
|
||
if (tooltipPercentageDisplayModes[tooltipPercentageDisplayOption] === undefined) { |
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.
This won't be necessary for userscript, since everything is in this script :)
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.
Hmm... are you recommending a switch? Because either way (extension or UserScript) we still need to 'convert' the tooltipPercentageDisplayOption
string from extConfig in to a format for the tooltip.
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.
No i meant everything will be defined unless user misuse! Undefined is used in extension for change events or first install/new feature update
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.
Ohh! I get what you mean. But I'm using === undefined
here to check if the string given by the user in tooltipPercentageDisplayOption
from extConfig can be found as a key in the tooltipPercentageDisplayModes
dict :). If the user inputs anything other than a correct option tooltipPercentageDisplayOption
is set to "classic"
(the default value).
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.
Yeah that's what I'm saying, you don't need it, because tooltipPercentageDisplayOption
is set at the beginning, inside extConfig
if you use the other pr. If it's undefined the switch statement should get into default.
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.
Actually i just read your code again. I would recommend against assigning strings for all options for every video, because you only need to see one. It's using a lot of computing power to process toLocaleString(), and the result will just get wasted.
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.
Right, I missed that somehow, guess I should use a switch.
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.
Fixed!
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.
Cool feature
It was @Nightcaat's idea I just implemented it, but thank you 😄 |
Issue #211
I've had the same idea for some time, so after seeing @Nightcaat's issue I decided to work on this feature.