-
Notifications
You must be signed in to change notification settings - Fork 898
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
Add hide channel option directly to More Options menu #4228
Add hide channel option directly to More Options menu #4228
Conversation
I think the new option should be added to the end with a separator |
Good work @MitchelPaulin! On top of @PikachuEXE's suggestions, could you also add an "Unhide Channel" option for when the channel is hidden? That may sound silly, but hidden channel videos can still show up in other places like Subscriptions, the Channel page, and Playlists. |
Head branch was pushed to by a user without write access
Moved it into its own divider at the bottom and added the ability to show hidden channels, good suggestions. Not sure how I feel about the confirmation popup, I feel like it might just get in the way more than anything. But if its a blocker I can add it. |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
I don't think it's strictly needed because you can always see and remove those channels from your Distraction Free Settings. Especially given that there's a toast letting you know what you did if you accidentally clicked it. @PikachuEXE Second thoughts? |
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? Forgot to approve after review
hideChannel: function(channelName) { | ||
const hiddenChannels = JSON.parse(this.$store.getters.getChannelsHidden) | ||
hiddenChannels.push(channelName) | ||
this.updateChannelsHidden(JSON.stringify(hiddenChannels)) | ||
|
||
showToast(this.$t('Channel Hidden', { channel: channelName })) | ||
}, |
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.
Please change this pull request to use the channel ID, not the name. Blocking by name is unreliable as the channel owner can change the channel name, so you would have to block it again when that happens, but the ID never changes, so it will continue to work even when the name changes.
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 went with the name since the Ids were very unreadable, if #4230 merges though that's no longer a concern, I can switch it to Ids
Head branch was pushed to by a user without write access
} | ||
) | ||
|
||
const hiddenChannels = JSON.parse(this.$store.getters.getChannelsHidden) | ||
const channelShouldBeHidden = hiddenChannels.some(c => c === this.channelName || c === this.channelId) |
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.
AFAIK channelName is not a thing anymore
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.
Would we not need the channel name checked here to support anyone that has existing legacy blocks via channel name? Or were they all migrated to reference the channel Id?
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 could be missing something but
If im looking at the conversations in #4230 i think everything is based on channel ID now
it was also mentioned that we should change this PR to channel ID only when that one was merged #4230 (comment)
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 was just thinking of what would happen to a user who has blocked a channel by name then updates freetube, You can no longer make new blocks by channel name but even in #4230 to me it looks like existing channel name blocks are still respected https://github.com/Benjababe/FreeTube/blob/7272fc478c9c8a5d6a17f54660a1e93ba20ca943/src/renderer/components/ft-list-video-lazy/ft-list-video-lazy.js#L72
Or is it understood that updating freetube means your existing settings might no longer work
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.
Ah ok, scrolling through the other PR it does look like we are fine just not respecting legacy blocks #4230 (comment)
In that case I can remove the channel name check
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.
In the latest nightly this isnt a thing
Head branch was pushed to by a user without write access
I see |
Head branch was pushed to by a user without write access
Yes, missed a spot, the rest of the uses are for the toast message though, so those need to stay |
Getting this issue on
|
Looks like an error caused by invalid YAML. The newly added strings with the curly bracket at the start probably need to be quoted. |
Head branch was pushed to by a user without write access
Thank you for your contribution! sorry for the long back-and-forwards on this. |
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 tested after merging development
into this branch otherwise it might not work properly
All seems fine
* development: Update to silently allow channel links (with IDs) (FreeTubeApp#4347) Add hide channel option directly to More Options menu (FreeTubeApp#4228) Translated using Weblate (Chinese (Simplified)) Translated using Weblate (Spanish) Make landing page configurable (FreeTubeApp#4200) Bump @silvermine/videojs-quality-selector from 1.3.0 to 1.3.1 (FreeTubeApp#4362) Bump electron from 27.0.4 to 27.1.0 (FreeTubeApp#4363) Bump the eslint group with 1 update (FreeTubeApp#4360) Bump actions/github-script from 6 to 7 (FreeTubeApp#4361) add image to poll & updated quiz style (FreeTubeApp#4318) Move hideOutlines to the utils store instead of using provide/inject (FreeTubeApp#4246) Translated using Weblate (Kurdish (Central)) Translated using Weblate (English (United Kingdom)) Translated using Weblate (French) Translated using Weblate (German) Added translation using Weblate (Kurdish (Central)) Translated using Weblate (Slovak) # Conflicts: # src/renderer/components/ft-list-video/ft-list-video.js
Add hide channel option directly to More Options menu
Pull Request Type
Related issue
closes #3051
closes #4037
Description
Piggybacks off the feature that was implemented in #2849 to add an option directly to the "More Options" menu to hide a channel. Options will show up in the "Distraction Settings" afterwards and can be removed from there.
Screenshots
Hide if not hidden
Show if hidden
Testing
Selected the option in the context menu, the video is immediately filtered and a toast is thrown up to confirm it was hidden. Then I checked the "Distraction Settings" and confirmed the channel is there.
Desktop
Additional Info
I decided not to go with "Hide Recommendation" like mentioned in the original issue as there really isn't recommendations, just trending.