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

Option to hide videos from certain channels #2849

Conversation

petaded
Copy link
Contributor

@petaded petaded commented Nov 9, 2022

Option to hide videos from certain channels

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

partial implementation of #95

Description

This PR adds a new setting under distraction free settings to hide specific channels.
You can write either the channel name or the channel id into the input box - multiple channels allowed.

This also adds new UI input type ft-input-tags which is used as the input method for this new option.

Videos and playlists from this channel ( and the channel itself ) will no longer show up in a search, recommended or trending.
Note: the box is case sensitive and must be an exact match to the channel name.

Screenshots

The new setting ( see bottom ):
image
Option box with multiple channels entered
image

Tooltip
image

Trending with empty box
image
Trending with shane tag in hide channel box
image
Tending after having shane and A24 tags in hide channels box
image

Testing

Tested the following:

  • since this touched the livestream filtering code
    • Search "music live" with hide livestreams as false, validate live streams still show
    • Search "music live" with hide livestreams as true, validate live streams no longer show
  • Search "ltt" with empty Hide channels, ltt videos show
  • Search "ltt" with "Linus Tech Tips" in hide channel box, ltt videos hidden + channel also hidden
  • Search "ltt" with "Linus Tech Tips" and "LMG Clips" tags in hide channel box, ltt and lmg clips videos hidden
  • Search "ltt" with "UCXuqSBlHAE6Xw-yeJA0Tunw" in hide channel box ( ltt channel id), ltt videos hidden

Desktop

  • OS: Manjaro
  • OS Version: 22
  • FreeTube version: 0.18.0

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 9, 2022 19:13
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 9, 2022
@PikachuEXE
Copy link
Collaborator

Idea wise it's useful
But UI/UX wise it's inconsistent with other "distraction free" settings
(It might even deserve a dedicated section if we would have more similar settings in the future)

  • There is no way I can tell it's about hiding channels
  • FT has no previous input for multiple text values, the UI for such new input type requires some discussion to agree on
    But having a text box to have value separated by a separator is certainly unacceptable for me

I imagine the UI at least look like the following
image

https://dribbble.com/shots/5970633-Add-Tags

It should handle many values (10+) and probably also mobile friendly
But let's make it at least work for desktop for the 1st version

This is only my opinion so would need more people to comment on this

@petaded
Copy link
Contributor Author

petaded commented Nov 11, 2022

@PikachuEXE I would completely agree.

I posted it as it was because:

  1. I happened to want this feature recently so coded something up for myself, was sharing this in case its useful for someone else
  2. I have not done any javascript, electron, vue programming before, so was trying to limit the scope of my changes

However I think you are right, it shouldn't be included as is, the current design is not intuitive.
I am happy for anyone else to add to this PR otherwise I'll have a look into styling a new input type when I get some more time.

@miangraham
Copy link
Contributor

It might even deserve a dedicated section if we would have more similar settings in the future

I've been thinking along similar lines as I'm considering adding a "never show me shorts" setting, which could just as easily be a "filter by title keywords" setting since YT just has the tag sitting in the title.

It would make sense for video-hiding settings to move to a new category rather than sitting in Distraction Free Settings. It's a consistent structural divide between options that toggle dedicated UI components versus options that filter items from lists. And it would do a bit to trim Distraction Free's massive wall of switches.

If my other PR lands soon I may try pushing this one for a bit. I'm pretty new to Vue but I imagine there are tag list component examples floating around.

@petaded
Copy link
Contributor Author

petaded commented Nov 13, 2022

I was looking at this a bit today. have made a "ft-input-tags" component - have not yet done the logic was just sorting the look.
Will update the code when I have it working.

Currently looks like this :
image
image

<ft-input-tags
          :title="$t('Settings.Distraction Free Settings.Hide Channels Title')"
          :placeholder="$t('Settings.Distraction Free Settings.Hide Channels Placeholder')"
          :show-action-button="true"
          :value="hideSpecificChannels"
        />

@miangraham
Copy link
Contributor

Looks great. Thanks for posting to save the duplicate work. Will definitely want to use that in the future myself.

@PikachuEXE
Copy link
Collaborator

@petaded Looks good so far!
Please let us know when done (enough for testing)

auto-merge was automatically disabled November 14, 2022 15:06

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 14, 2022 15:06
@petaded
Copy link
Contributor Author

petaded commented Nov 14, 2022

Have updated PR.
Added a new ft-input-tags input component. The new option now uses this component. The logic for hiding videos etc is still the same just the input method is updated.

Here is a short video of using it.

input_tags.mp4

If someone could test on windows that would be useful ( I don't have access to a windows computer )

I still have it under distraction free settings but only because I am not sure what/where the option should go, some more input on this would be welcome.

auto-merge was automatically disabled November 14, 2022 18:10

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 14, 2022 18:11
@PikachuEXE
Copy link
Collaborator

The new UI element looks and works fine in simple test

Though I am not sure what value I should put into it (Channel name can change but I guess rarely?)
Does channel ID work? Does partial name work?
I can never tell until I test it or look at the code and this is bad for UX.

For the position I would say put it as a separate box below the columns of switches (so the width = all columns)

auto-merge was automatically disabled November 15, 2022 11:00

Head branch was pushed to by a user without write access

@petaded
Copy link
Contributor Author

petaded commented Nov 15, 2022

@PikachuEXE Thank you for the feedback.
I have added an optional tooltip parameter to the ft-input-tags component. Then added some tooltip text to the distraction free setting see picture below:
image
I also updated the placeholder text to be "Channel Name or ID" to make it more obvious without reading the tooltip that you can also use a channel id.

Also as suggested have moved the setting to below the other toggle switches.
image

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 15, 2022 11:00
auto-merge was automatically disabled November 15, 2022 11:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 15, 2022 11:23
@ChunkyProgrammer
Copy link
Member

It's not very accessible at the moment (labels should be linked to the inputs)

@petaded
Copy link
Contributor Author

petaded commented Nov 15, 2022

Thanks @ChunkyProgrammer for having a look, however I don't quite understand what you mean by labels should be linked to inputs - I'm guessing you are talking about the code?

Do you have specific suggestions of what I could change to improve it?

@ChunkyProgrammer
Copy link
Member

Just a note: it's not really that accessible. Labels should have 'for' and

Thanks @ChunkyProgrammer for having a look, however I don't quite understand what you mean by labels should be linked to inputs - I'm guessing you are talking about the code?

Do you have specific suggestions of what I could change to improve it?

Sorry, I should have explained it better. The HTML label elements should have a for attribute that links it to an input (via the id).
Example: <label for="element_id">

To test: Clicking on the label should focus the input element.

Here is some more documentation on it if you are interested: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label#try_it

The looks really good so far 😄
Another issue that could arise is if you are blocking a channel that has a semicolon in it's name.
Example: https://www.youtube.com/@esemicolonr

@PikachuEXE
Copy link
Collaborator

I agree on the semicolon separator issue
Need to use another "won't be used on channel name" char
But I have no idea what to use :)

@github-actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

}
}
}
return true
Copy link
Member

@absidue absidue Nov 24, 2022

Choose a reason for hiding this comment

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

the previous check hid the item when data is undefined, this implementation doesn't

Copy link
Contributor

@miangraham miangraham Nov 24, 2022

Choose a reason for hiding this comment

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

I believe the conflict resolutions in the rebase below will resolve this as well. See 4d618e3, L59.

Looks fixed after petaded's merge commit!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is fixed with the merge with latest development

@miangraham
Copy link
Contributor

miangraham commented Nov 24, 2022

@petaded I ran a rebase for this on development after my conflicting changes were merged. Tested that my new setting and yours still work afterward. Result over here, feel free to grab/squash:

https://github.com/miangraham/FreeTube/tree/add_option_to_hide_channels

auto-merge was automatically disabled November 24, 2022 17:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 24, 2022 17:23
@github-actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@petaded
Copy link
Contributor Author

petaded commented Nov 24, 2022

Hi all,

have merged latest development in including @miangraham changes.
The merge has fixed the issue @absidue points out and the conflicts.

Would have used your provided merge @miangraham but i'd already done and pushed my merge when i saw your comment. thanks for the help though.

@miangraham
Copy link
Contributor

Ran a quick test with npm run dev post-merge and theirs/ours sides both work fine. Channels hidden by name, premieres hidden by flag. LGTM!

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 27, 2022
Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

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

Lgtm

@FreeTubeBot FreeTubeBot merged commit f33f142 into FreeTubeApp:development Dec 17, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 17, 2022
@BarbzYHOOL

This comment was marked as abuse.

@absidue
Copy link
Member

absidue commented Jan 13, 2023

This was merged on the 17th of December, the latest release 0.18.0 was on the 2nd of November...

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

Successfully merging this pull request may close these issues.

7 participants