-
-
Notifications
You must be signed in to change notification settings - Fork 291
feat: adding setting to show/hide user ratings #1426
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
feat: adding setting to show/hide user ratings #1426
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I'm confused, is this not what custom CSS is for? Why would you want to remove ratings anyways? Just seems like bloat for the app and settings menu to me but excuse me if I'm just misinformed |
|
I come from the Navidrome where you can hide the ratings feature from the UI with an environment variable. I had it turned on initially as it is the default but me and the other person that use the instance really disliked it so we disabled it and never turned back. Now that we're using Feishin as our client we wanted to do the same. I decided to give a shot at implementing it since I saw other issues showing interest in also disabling the ratings feature here. If this just feels like bloat and doesn't have enough traction to be a feature that's totally fair! I will just go with the custom css solution, but it would be nice to just be able to open the settings and disable it, especially for users that aren't very tech savvy. |
|
Chiming in to the discussion, I believe this is a nice feature to have and that Custom CSS should only be recommended as a temporary band-aid fix. Ultimately, I want to recommend Feishin to non-technical people and telling them to do something with Custom CSS is unacceptable and greatly hinders adoption. |
|
I understand the part about custom CSS but I am yet to see a single reason why there is any need to hide ratings in the first place which is why I simply deemed this as bloat and something that doesn't need to exist. If you don't use ratings you can just ignore it, I don't see why we need to be storing and displaying yet another option and spending the CPU time to decide whether or not to render it |
| const columns = useMemo(() => { | ||
| if (showRatings) { | ||
| return table.columns; | ||
| } else { | ||
| return table.columns.filter((column) => column.id !== 'userRating'); | ||
| } | ||
| }, [table, showRatings]); |
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 would remove this change from all table column instances, as the columns are already configurable to remove the rating column if not desired.
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 reverted all the changes on the table columns. I ended up having to change how the cards choose to display the rating stars as well. My initial approach was to remove the onRating completely from all controls but this causes weirdness when the user had the ratings hidden on the settings but choose to display the ratings on any of the tables because the ratings would show up and be interactive on hover but clicking on it would have no effect and I found that interaction to be weird.
3088332 to
9e096ce
Compare
This PR is for adding a setting for showing or hiding the user ratings feature. I was about to open an issue about it but I saw a few others that were already mentioning it so I decided to give it a try.
I think I have covered all the views but please do let me know if I missed something.
These issues are related to this: #1385 #1398