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

Update 'Only Show Latest Video for Each Channel' setting to handle a custom number of videos per channel #5901

Open
wants to merge 5 commits into
base: development
Choose a base branch
from

Conversation

c-ciobanu
Copy link

@c-ciobanu c-ciobanu commented Oct 20, 2024

Update 'Only Show Latest Video for Each Channel' setting to handle a custom number of videos per channel

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #5265

Description

Updated the Only Show Latest Video for Each Channel setting to now handle a custom number of videos per channel to be shown.
Added a new variable to keep track of the number so the user can change it if the option is enabled.
I didn't change the existing setting so this way this feature is retro compatible.

Screenshots

simplescreenrecorder-2024-11-16_12.22.51.mp4

image

Testing

Enabled the feature and changed the number different times while checking that the Videos, Shorts, and Live tabs showed the correct amount of videos for each channel.

Desktop

  • OS: Linux Mint
  • OS Version: 21
  • FreeTube version: 0.21.3

Additional context

I didn't remove the old localisation string, is it removed automatically or do I need to do it.

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 20, 2024
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 20, 2024 13:51
Copy link
Collaborator

@kommunarr kommunarr left a comment

Choose a reason for hiding this comment

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

I haven't tested this yet, but good work! One potential suggestion: thoughts on having the label change to Limit the number of videos displayed for each channel to when the toggle is enabled and making the input an inline 2-digit-wide one? That way it could read out like a sentence.

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

I didn't remove the old localisation string, is it removed automatically or do I need to do it.

Its not automatically, you need to remove it.

@c-ciobanu
Copy link
Author

c-ciobanu commented Oct 20, 2024

I haven't tested this yet, but good work! One potential suggestion: thoughts on having the label change to Limit the number of videos displayed for each channel to when the toggle is enabled and making the input an inline 2-digit-wide one? That way it could read out like a sentence.

I personally prefer something like this:
image
to this:
image

Let me know what you think.

@c-ciobanu
Copy link
Author

I didn't remove the old localisation string, is it removed automatically or do I need to do it.

Its not automatically, you need to remove it.

I imagine I have to remove it from all the files not just static/locales/en-US.yaml, right?

@kommunarr
Copy link
Collaborator

kommunarr commented Oct 20, 2024

I personally prefer the latter (with a 1em horizontal margin), although either way is fine from a general styling standpoint as long as other contributors feel the same. My main concern is that if we were to add a new toggle to that column, it would be visually unclear if the input correlated to the top toggle or the bottom one. That concern could possibly be assuaged by reducing the vertical margin between the two controls, but I'm not sure, as it's already so small between the toggles.

On that note, you'll also need an aria-labelledby (example) on the base input pointing to the toggle's title to ensure that this is accessible for users with screen readers.

auto-merge was automatically disabled October 22, 2024 13:14

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 22, 2024 13:15
@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 Oct 22, 2024
Copy link
Contributor

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

auto-merge was automatically disabled October 22, 2024 13:26

Head branch was pushed to by a user without write access

@c-ciobanu c-ciobanu force-pushed the limit-number-of-videos-per-channel branch from eef0111 to ad8b129 Compare October 22, 2024 13:26
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 22, 2024 13:27
Copy link
Contributor

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

auto-merge was automatically disabled October 22, 2024 13:40

Head branch was pushed to by a user without write access

@c-ciobanu c-ciobanu force-pushed the limit-number-of-videos-per-channel branch from ad8b129 to e183f1d Compare October 22, 2024 13:40
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 22, 2024 13:40
/>
<div class="onlyShowLatestFromChannel">
<ft-toggle-switch
:id="'onlyShowLatestFromChannel'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:id="'onlyShowLatestFromChannel'"
id="onlyShowLatestFromChannel"

/>
<ft-input
v-if="onlyShowLatestFromChannel"
:placeholder="'1'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:placeholder="'1'"
placeholder="1"

:show-action-button="false"
input-type="number"
:value="onlyShowLatestFromChannelNumber"
:aria-labelledby="'onlyShowLatestFromChannel'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:aria-labelledby="'onlyShowLatestFromChannel'"
aria-labelledby="onlyShowLatestFromChannel"

Copy link
Member

Choose a reason for hiding this comment

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

Please don't use nested CSS unless there is absolutely no other way, as it breaks Vue's scoped CSS functionality, in this case a class would probably be a lot more suitable than doing some nth-child stuff. That would also allow you to use a plain CSS file.

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

I didn't remove the old localisation string, is it removed automatically or do I need to do it.

Its not automatically, you need to remove it.

I imagine I have to remove it from all the files not just static/locales/en-US.yaml, right?

Thats correct

Copy link
Member

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

Choose a reason for hiding this comment

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

  • Lets only allow positive numbers >= 1

image

  • Also we should set the limit to 30 as we dont display more than that
  • I dont like how all the settings move when you enable this
VirtualBoxVM_08l4jeu4gq.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 23, 2024
@absidue absidue added the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 23, 2024
@c-ciobanu
Copy link
Author

c-ciobanu commented Oct 23, 2024

  • Lets only allow positive numbers >= 1

image

  • Also we should set the limit to 30 as we dont display more than that
  • I dont like how all the settings move when you enable this

VirtualBoxVM_08l4jeu4gq.mp4

Seeing how the input and the settings work right now I don't think it's possible to have a limit enforced nicely right now since there is no validation at all.

For the alignment something like this would be better:

simplescreenrecorder-2024-10-23_16.23.50.mp4

Layout not moving and it looks the same on lower resolutions.

Maybe instead of having an input a slider would be better?

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

Hmm i think thats a good suggestion to go the slider route. Also makes it more uniform with the other settings

@ChunkyProgrammer ChunkyProgrammer removed the DO NOT MERGE UNTIL AFTER RELEASE Do not merge before the next release as this is not a bug fix label Oct 27, 2024
@kommunarr kommunarr added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Oct 27, 2024
Copy link
Contributor

github-actions bot commented Nov 2, 2024

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

auto-merge was automatically disabled November 16, 2024 11:23

Head branch was pushed to by a user without write access

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 16, 2024 11:23
auto-merge was automatically disabled November 16, 2024 11:28

Head branch was pushed to by a user without write access

@c-ciobanu c-ciobanu force-pushed the limit-number-of-videos-per-channel branch from 922ef31 to dd2fc9c Compare November 16, 2024 11:28
Copy link
Contributor

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

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.

Works well but I will let you guys approve

Copy link
Contributor

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

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.

[Feature Request]: Only Show Latest x Videos for Each Channel
6 participants