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

Add channel avatars to Subscription tab #5409

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

tzwel
Copy link

@tzwel tzwel commented Jul 15, 2024

Add channel avatars to Subscription tab

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

This PR adds channel avatars beside names in the subscription feed. Supports both APIs

Screenshots

Subscriptions tab
image

Search results (no thumbs displayed as discussed here)
image

Testing

The pull request has been tested on many channels and properly fallbacks to not displaying a thumb when one isn't available.

Desktop

  • OS: Windows
  • OS Version: 2004
  • FreeTube version: v0.21.1 Beta

Additional context

I'm happy to fix anything as I come from Svelte background and I haven't really touched Vue.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 15, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) July 15, 2024 08:09
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Strange spacing in list view
image

@@ -92,6 +93,7 @@ export default defineComponent({
title: '',
channelName: null,
channelId: null,
channelThumbnail: null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not PR specific, I got this opinion for the entire app
channelThumbnail always looks like a variable with an object to me

Suggested change
channelThumbnail: null,
channelThumbnailUrl: null,

@@ -221,6 +223,10 @@ export default defineComponent({
return `https://www.youtube-nocookie.com/embed/${this.id}`
},

profileSubscriptions: function () {
return deepCopy(this.$store.getters.getActiveProfile.subscriptions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need deepCopy here?

Comment on lines +712 to +716
this.channelThumbnail = thisChannel?.thumbnail?.replace(/=s\d+/, '=s35')

if (this.backendPreference === 'invidious' && this.channelThumbnail) {
this.channelThumbnail = youtubeImageUrlToInvidious(this.channelThumbnail, this.currentInvidiousInstance)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to put URL into a variable first, performing ops like replace, youtubeImageUrlToInvidious then assign the final value to this.channelThumbnail for performance

Comment on lines +31 to +38
.channelThumbnail {
border-radius: 999px;
display: inline-block;
max-inline-size: 24px;
max-block-size: 24px;
margin-block-end: 0 !important;
margin-inline-end: 0.5rem;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you are trying to override the value in .ft-list-item.grid but I think it's better to avoid using !important when possible

Suggested change
.channelThumbnail {
border-radius: 999px;
display: inline-block;
max-inline-size: 24px;
max-block-size: 24px;
margin-block-end: 0 !important;
margin-inline-end: 0.5rem;
}
.channelThumbnail {
border-radius: 999px;
display: inline-block;
max-inline-size: 24px;
max-block-size: 24px;
margin-inline-end: 0.5rem;
@at-root {
// Required to override style from ft-list-item
.ft-list-item.grid & {
margin-block-end: 0 !important;
}
}
}

What existing style is for:
image

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 16, 2024
Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@tzwel
Copy link
Author

tzwel commented Aug 14, 2024

I'm still here. Life is getting in the way

@absidue
Copy link
Member

absidue commented Aug 14, 2024

We should probably remove the closes text from this issue, as this pull request only implements the feature for the subscriptions page.

Adding support for displaying the channel thumbnails in the search results, that are already in the search API response coming from YouTube (we don't need a cache for it, which is what you seemed to be saying was needed in the discussion you linked), can either be done in this pull request or in a separate one.

Edit: removed the closes text, feel free to add it back if you decide to add the search results to this pull request too.

Copy link
Contributor

This PR is stale because it has been open 28 days with no activity. Remove stale label or comment or this will be closed in 14 days.

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

Successfully merging this pull request may close these issues.

4 participants