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

Added trending toggle in client settings #2735

Merged
merged 7 commits into from
Apr 4, 2024

Conversation

nargacu83
Copy link
Contributor

@nargacu83 nargacu83 commented Mar 31, 2024

Hi! I've added a toggle in the client settings to disable trending videos in the home screen. Which resolves #2587.

image

Having both Show subscriptions on home screen and the new Show trending videos on home screen off results in an empty page, do we need to do something about this as mentioned in the issue?

Do we expand the number of subscriptions videos on the home page when this setting is disabled?

@moisout
Copy link
Member

moisout commented Apr 2, 2024

Hi, thanks a lot for your PR! Please give me a moment to check it out

@moisout
Copy link
Member

moisout commented Apr 2, 2024

Looks good! I would recommend adding the settings entry to the backend, so it can be synced to the user account.
Like for example in #2739

@moisout
Copy link
Member

moisout commented Apr 2, 2024

As for the empty homepage, maybe just show a little icon or something to indicate it hasn't failed loading. Or leave it blank, however you prefer :)

frozenduck and others added 2 commits April 2, 2024 22:55
* Added comments toggle in client settings

* hideComments toggle category change

---------

Co-authored-by: iyakushev <yakushevis@itsintellect.ru>
@nargacu83
Copy link
Contributor Author

As for the empty homepage, maybe just show a little icon or something to indicate it hasn't failed loading. Or leave it blank, however you prefer :)

Agreed, i think i'll just move the search bar and the logo to the center of the page like Invidious does, if that's ok.

image

@moisout
Copy link
Member

moisout commented Apr 2, 2024

Thats a cool idea!

@nargacu83
Copy link
Contributor Author

Done! Let met know if you see anything odd.

@@ -28,7 +27,7 @@ const { posAbsolute, topPositionPx } = useHeaderScroll();
left: 0;
display: flex;
flex-direction: row;
justify-content: space-between;
justify-content: right;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was needed because when we hide the Logo and the MainSearchBox, the UserMenu was on the left side of the header bar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that moving the Logo to it's own component would help for any use case outside of the header bar.

@moisout
Copy link
Member

moisout commented Apr 3, 2024

Thanks a lot, I'll test it aso soon as I can

@moisout
Copy link
Member

moisout commented Apr 3, 2024

Looks great, the only thing i noticed is, the search bar and logo doesn't reappear when you are on a different page from the homepage
image

@nargacu83
Copy link
Contributor Author

Looks great, the only thing i noticed is, the search bar and logo doesn't reappear when you are on a different page from the homepage

oh yes, i forgot about that issue.

@moisout
Copy link
Member

moisout commented Apr 3, 2024

Looks great, the only thing i noticed is, the search bar and logo doesn't reappear when you are on a different page from the homepage

oh yes, i forgot about that issue.

You could maybe recognize the current page by using const route = useRoute(); and then using route.name

@nargacu83
Copy link
Contributor Author

Looks great, the only thing i noticed is, the search bar and logo doesn't reappear when you are on a different page from the homepage

oh yes, i forgot about that issue.

You could maybe recognize the current page by using const route = useRoute(); and then using route.name

Sorry, i didn't see your message. I just sent a fix, let me know if it's good.

@moisout
Copy link
Member

moisout commented Apr 3, 2024

Looks good, the only thing i would change is moving the useRoute into script setup, so it only gets called once

@nargacu83
Copy link
Contributor Author

Done!

@moisout
Copy link
Member

moisout commented Apr 4, 2024

Looks good to me, I'll merge

@moisout moisout merged commit b3113a5 into ViewTube:development Apr 4, 2024
5 checks passed
@nargacu83 nargacu83 deleted the trending-toggle branch April 4, 2024 16:54
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.

Cleaner Interface: Remove Trending Videos and Recommended Videos
3 participants