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

Shorts tab on channel page #3093

Closed

Conversation

KarateCowboy
Copy link

@KarateCowboy KarateCowboy commented Jan 18, 2023

Add 'Shorts' tab to the Channel page

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Description

Adds the Shorts tab for a channel to the Channel Page. Allows for playing via direct or invidious api

Screenshots

Before:

before

After:

after

Testing

  1. Find a channel which has Shorts and Subscribe to it.
  2. View the Channels page, then click on that channel
  3. The new "Shorts" tab should be visible. Click it
  4. There should be a grid with Shorts in it.
  5. Click any of the shorts. You should be taken to the Watch page and be able to watch the Short

@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Jan 18, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 18, 2023 20:58
@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jan 19, 2023
@github-actions
Copy link
Contributor

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

auto-merge was automatically disabled January 19, 2023 04:48

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 19, 2023 04:48
@absidue
Copy link
Member

absidue commented Jan 19, 2023

This PR doesn't actually implement the feature request that you mention in the Related issue section. As mentioned in #2476 (comment) that feature request is actually for the infinitely scrolling shorts tab, although there is an unrelated discussion towards the end of that thread that mentions the shorts tab on the channels page.
I'll remove it from the pull request description for you, so that it stays open until someone actually implements it.

@@ -393,7 +393,7 @@ export default Vue.extend({
this.publishedText = this.data.publishedText
}

if (typeof (this.data.publishedText) !== 'undefined' && this.data.publishedText !== null && !this.isLive) {
if ('publishedText' in this.data && !!this.data.publishedText && !this.isLive) {
Copy link
Member

Choose a reason for hiding this comment

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

This change can be simplified to this:

Suggested change
if ('publishedText' in this.data && !!this.data.publishedText && !this.isLive) {
if (this.data.publishedText && !this.isLive) {

@KarateCowboy
Copy link
Author

KarateCowboy commented Jan 19, 2023

Hi,

So, in issue #2476 there is quite a bit going on.

  • The opening post by a deleted user asks for a dedicated shorts tab but is ambiguous beyond that. We obviously cannot ask for clarification.
  • Then @ChunkyProgrammer speculates that the user may have wanted an infinite scrolling list of some type.
  • Then we see @efb4f5ff-1298-471a-8973-3d47447115dc saying there should be a shorts tab that is just like the subscriptions tab. All shorts, from all channels
  • I thought there was also a user asking for a shorts tab on the channels page. Is that what you removed?
  • Finally, there is @ChunkyProgrammer commenting that there should be a toggle to disable both shorts and streams

I see content for several stories in there:

  1. A shorts tab on the channel page
  2. a shorts page which includes aggregated shorts from all subscriptions
  3. a streams tab on the channel page
  4. an infinite scrolling loop of shorts with autoplay
  5. the ability to disable shorts
  6. the ability to disable streams

How about this for a plan of action: we use this PR as an MVP to get shorts, at all, out the door(item 1 on the list)? I can go and create separate stories for the rest, and flesh out the details plus input. Let me know your thoughts, everyone.

@absidue
Copy link
Member

absidue commented Jan 21, 2023

The only thing I edited was the description of this pull request, to remove the reference to a feature request for a different feature (See the screenshot below).

Why am I so sure that the original feature request is unrelated to the shorts tab on the channel page?
That feature request was opened in August, YouTube only split the videos tab up into the videos, shorts and live tabs in October/November. Additionally the requester asked for a dedicated shorts tab and Chunky pointed out that the YouTube has a dedicated infinitely scrollable shorts tab (visit https://www.youtube.com and click shorts in the sidebar), so it's highly likely that the requester meant that.

This pull request only implements the discussion that is going on below that feature request, not the actual feature request itself. I removed your "implements #number" text, so the feature request stays open until it is implemented.

I'm sorry that my words and actions came across as attacks, that wasn't my intention. My intention was just to point out that the feature that this pull request implements, is not the one in the feature request that you linked, so I removed your link to it. This pull request is completely fine (well a few code changes are needed before it gets merged, like fixing the code conflicts) as a pull request that implements the shorts tab on the channel page.

pr-edit

@KarateCowboy
Copy link
Author

KarateCowboy commented Jan 22, 2023

Hi absidue

Forgive me if the verbosity of my last post made it seem like I was being defensive. I did not interpret what you wrote as combative. There is much to this project which I am still learning: what code does what, who is active and coding, and who is keeping track of what. Because of that I decided to be very detailed about what I observe and am thinking. Also, I am excited and charged up to help out, so there is that energy.

What you are saying about Youtube splitting pieces versus the creation of the feature request makes perfect sense. I totally agree.

I will clean up the code conflicts. There are two outstanding things I see where I need some advise:

  1. What do I need to do about i18n for the features, if anything?
  2. My code is pointing to a git commit, per @ChunkyProgrammer 's advice. Do I need to wait for a new release, or is it ok to commit with the package.json pointing at a git branch/hash?

After that, what is the review, test, and release process?

auto-merge was automatically disabled January 22, 2023 21:28

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 22, 2023 21:28
auto-merge was automatically disabled January 22, 2023 21:30

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 22, 2023 21:30
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.

  1. You are missing a string for 'This channel does not currently have any shorts' in the en-US locale file. Other than that i18n looks good.
    image

The translations for other languages are done through Weblate and if a translation is missing in a language, it will fallback to en-US 😄

  1. Pointing to the git branch should be fine for now (we'll be using YouTube.js in FreeTube's next release instead of yt-channel-info)

"Also, while the request mentioned the ability to hide shorts, I could find no such toggle. Perhaps I could add that on to this." sorry, I just didnt remember off the top of my head if that functionality was already implemented or not 😄 if you would like to add it, you can do it in a separate PR.

If you'd like to add the ability to load more shorts to this PR:
a continuation parameter can be added to the invidious call or you can call getChannelVideosMore with the shorts continuation

Let me know if you have any more questions. Thanks for opening a PR!

src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
src/renderer/views/Channel/Channel.js Outdated Show resolved Hide resolved
src/renderer/views/Watch/Watch.js Outdated Show resolved Hide resolved
static/locales/en-US.yaml Show resolved Hide resolved
auto-merge was automatically disabled January 28, 2023 20:22

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 28, 2023 20:22
@KarateCowboy
Copy link
Author

KarateCowboy commented Jan 28, 2023

I updated and took all of @ChunkyProgrammer 's suggestions. It appears that additional shorts are automatically loading as I scroll down the page. Hopefully it works the same for you. The only outstanding issue I see is the conflict in package.json between the released version of yt-channel-info. My code is pointing directly to a git commit.

Thanks for opening a PR!

Thank you for creating this application. And, thank you for facilitating my contribution.

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Jan 28, 2023

Would you be able to resolve the merge conflict as well @KarateCowboy ?

@KarateCowboy
Copy link
Author

@ChunkyProgrammer Sure thing. It looks like I missed the part where you said it is OK to point to the git commit.

@absidue Sure thing. To replicate the error, remove the extra conditional. Then subscribe to the channel Julia Stunts. Then go to that channel via "Channels > Julia Maggio". Then click the shorts tab. Then the Short "Devil Hunters@WholeWheatPete @itsKingChris & @StellaChuu".

@github-actions
Copy link
Contributor

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

package.json Fixed Show fixed Hide fixed
auto-merge was automatically disabled January 28, 2023 20:45

Head branch was pushed to by a user without write access

@KarateCowboy KarateCowboy requested review from ChunkyProgrammer and absidue and removed request for PikachuEXE and ChunkyProgrammer February 8, 2023 19:41
@efb4f5ff-1298-471a-8973-3d47447115dc

@KarateCowboy could u please upload the screenshots direct on GitHub instead of Dropbox

@KarateCowboy
Copy link
Author

@KarateCowboy could u please upload the screenshots direct on GitHub instead of Dropbox

Done

@efb4f5ff-1298-471a-8973-3d47447115dc

Noticed that the view count is off

@KarateCowboy
Copy link
Author

Noticed that the view count is off

I looked and that's actually what is coming through from yt-channel-info. if you console.log the response in src/renderer/helpers/api/index.js you can see:

console_and_view

And the same Short on YT:
yt_source

@absidue
Copy link
Member

absidue commented Feb 10, 2023

Could you please add the new tab to the tabInfoValues array on the channel page. That array is used so you can cycle the focus through the tabs by pressing the left and right arrow keys.

@ChunkyProgrammer
Copy link
Member

ChunkyProgrammer commented Feb 12, 2023

PS: This YouTube.js stuff --- how close to arrival is it?

The current plan:
Once LuanRT/YouTube.js#309 gets merged then #3143 will need some updates + testing + review and then the youtubei.js migration will be merged when approved.

@KarateCowboy
Copy link
Author

PS: This YouTube.js stuff --- how close to arrival is it?

The current plan: Once LuanRT/YouTube.js#309 gets merged then #3143 will need some updates + testing + review and then the youtubei.js migration will be merged when approved.

Understood. I will keep an eye on that and then update accordingly.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Feb 27, 2023
@github-actions
Copy link
Contributor

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

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Mar 2, 2023

@KarateCowboy #3143 is merged :)

@KarateCowboy
Copy link
Author

@KarateCowboy #3143 is merged :)

I'll get to it this weekend!

@efb4f5ff-1298-471a-8973-3d47447115dc

@KarateCowboy #3143 is merged :)

I'll get to it this weekend!

umm this weekend @KarateCowboy ?

@KarateCowboy
Copy link
Author

KarateCowboy commented Mar 11, 2023 via email

@absidue
Copy link
Member

absidue commented Mar 11, 2023

@KarateCowboy feel free to use #3273 as an inspiration (it adds support for the live tab).

@KarateCowboy
Copy link
Author

pulled in latest. hacking away a little tonight

@ChunkyProgrammer
Copy link
Member

Hi @KarateCowboy do you still plan on working on this? Note: invidious shorts should work once iv-org/invidious#3700 is merged

@KarateCowboy
Copy link
Author

Hi @KarateCowboy do you still plan on working on this? Note: invidious shorts should work once iv-org/invidious#3700 is merged

Hey hey. Thanks for the bump. I'll hit it up over the holiday weekend. Sunday night

@absidue
Copy link
Member

absidue commented Apr 6, 2023

Hi @KarateCowboy
Glad to hear that you are picking this up again :). To keep this pull request small and on topic, please leave the addition of a test framework and the refactor out of this pull request. If you want to add it, please do so in a different pull request, instead of cramming multiple things into one pull request. Keeping it simple, makes it easier and faster to review.

@efb4f5ff-1298-471a-8973-3d47447115dc

Closing, this is implemented in #3533

auto-merge was automatically disabled May 15, 2023 10:23

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants