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.

Sign up for GitHub

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

Jump to bottom

Update most paginated places to auto load next page (except comments) #4565

Merged
merged 12 commits into from
Apr 16, 2024
Merged

Update most paginated places to auto load next page (except comments) #4565

merged 12 commits into from
Apr 16, 2024

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Jan 17, 2024 •
edited
Loading

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

#4521

Description

Make following places to auto load next page

  • every subscription tab
  • channel tabs with pagination
  • hashtag page
  • history page
  • search result page
  • user playlists page
  • single playlist page (remote & local playlists)

Comments already handled by #3352 so not handled by this new option

Screenshots

image

Testing

Test places above with pagination and scroll down to ensure next page auto loaded only if option enabled

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) January 17, 2024 01:12
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 17, 2024
@kommunarr
Copy link
Collaborator

This would make sense to be next to Comment Auto Load. Which by the way, should not be under Player Settings, which we currently have it at for some reason. So maybe move that too.

@PikachuEXE
Copy link
Collaborator Author

Update comment section to auto load next page when this new option is enabled

I don't see why Comment Auto Load should be in General section when the only place it affects is comment section in watch page which is next to player (strictly speaking the Up Next & playlist components are also outside the actual video player and we place options like Autoplay Playlists & Play Next Video in the same section)

@absidue
Copy link
Member

absidue commented Jan 17, 2024

Instead of duplicating the code across all components, it would probably be better to create a dedicated component for it and then reuse that in the other components. When it becomes visible it can emit an event and then the "parent" can use that to call it's own increaseLimit or loadMore function.

@PikachuEXE
Copy link
Collaborator Author

@absidue Done (except comment section with custom logic

@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 18, 2024
@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

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

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

Everything works like expected. I did notice that this doesnt work for creator playlists but i assume that is because of YT.

If yes on previous statement then we have change messaging in tooltip

@PikachuEXE
Copy link
Collaborator Author

I need an example of creator playlists
Also please suggest a new message (I think there might be more than 1 type of special playlists that might not work with this new feature in the future...)

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Feb 2, 2024 •
edited
Loading

Sorry for the delay, its not working for me on these playlists

Also wanted to chime in on the earlier topic about the placement of Comment Auto Load. Why not merge the Auto comment load and this setting into one and put it under General Settings as Infinite Scroll?

@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 Feb 2, 2024
@PikachuEXE
Copy link
Collaborator Author

Coz Comment Auto Load also loads the comment section, not just next page

@PikachuEXE
Merge branch 'development' into feature/auto-load-next-page
9861ad2 
* development: (92 commits)
  Make video info section more concise (FreeTubeApp#4338)
  Playlist performance improvements (FreeTubeApp#4597)
  ! Fix playlist type not passed when playing next/prev item in a user playlist (FreeTubeApp#4623)
  Properly localize playlist view and video counts (FreeTubeApp#4620)
  Translated using Weblate (Croatian)
  Translated using Weblate (German)
  Translated using Weblate (Croatian)
  Fix search bar handling of Invidious channel URLs (FreeTubeApp#4568)
  Local API: List related games in featured channels section (FreeTubeApp#4562)
  Workaround community post slider dependency incorrectly calculating its size (FreeTubeApp#4598)
  Add support for viewing movie trailers with local api (FreeTubeApp#4391)
  Bump the eslint group with 2 updates (FreeTubeApp#4616)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Bump electron from 28.1.4 to 28.2.0 (FreeTubeApp#4611)
  Translated using Weblate (French)
  Bump the eslint group with 4 updates (FreeTubeApp#4581)
  Bump lefthook from 1.6.0 to 1.6.1 (FreeTubeApp#4608)
  Bump marked from 11.1.1 to 11.2.0 (FreeTubeApp#4612)
  Bump webpack from 5.89.0 to 5.90.0 (FreeTubeApp#4610)
  ...
@PikachuEXE
* Implement auto load next page for single playlist page
26c90a0
@PikachuEXE
Copy link
Collaborator Author

Implemented auto load for single playlist page (remote & local playlists)

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc previously approved these changes Feb 6, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

I think merging the feature should happen but is out of scope here, LGTM

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 6, 2024

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.

@github-actions GitHub Actions
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

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

@PikachuEXE
Merge branch 'development' into feature/auto-load-next-page
7293722 
* development:
  Translated using Weblate (Czech)
  Translated using Weblate (Slovenian)
  Add search playlists with matching videos function (FreeTubeApp#4537)
  Translated using Weblate (Danish)
  Translated using Weblate (Danish)
  Translated using Weblate (Danish)

# Conflicts:
#	src/renderer/views/UserPlaylists/UserPlaylists.js
@github-actions GitHub Actions
Copy link
Contributor

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

@PikachuEXE PikachuEXE added the PR: waiting for review For PRs that are complete, tested, and ready for review label Mar 15, 2024
@PikachuEXE
Merge branch 'development' into feature/auto-load-next-page
020f991 
* development: (72 commits)
  Add support for CommentViews in video comments (FreeTubeApp#4806)
  Add support for LockupViews on the channel podcasts tab (FreeTubeApp#4767)
  Bump youtubei.js from 9.1.0 to 9.2.0 (FreeTubeApp#4836)
  Bump swiper from 11.0.7 to 11.1.0 (FreeTubeApp#4837)
  Bump the stylelint group with 1 update (FreeTubeApp#4835)
  Bump the eslint group with 1 update (FreeTubeApp#4834)
  Bump electron from 29.1.5 to 29.1.6 (FreeTubeApp#4838)
  Translated using Weblate (Estonian)
  Fix v-observe-visibility error when playlist items are updated (FreeTubeApp#4774)
  Fix extracting the subscriber count from channel PageHeader nodes (FreeTubeApp#4804)
  Translated using Weblate (Portuguese)
  Create empty subscriptions cache objects directly instead of cloning (FreeTubeApp#4814)
  Parse compact numbers without using floating point numbers to avoid accuracy issues (FreeTubeApp#4817)
  Fix the left arrow key not working on the first button in prompts (FreeTubeApp#4816)
  Fix the web build's manifest.json file getting included in the electron build (FreeTubeApp#4815)
  Move usingElectron from computed into data (FreeTubeApp#4810)
  Stop setting node modules path now that we bundle the modules (FreeTubeApp#4809)
  Translated using Weblate (Bulgarian)
  Bump express from 4.18.1 to 4.19.2 (FreeTubeApp#4812)
  Translated using Weblate (Portuguese (Brazil))
  ...
@kommunarr
Copy link
Collaborator

This does NOT handle video comment auto load

Looking back at this, are we certain that maintaining this level of granularity is worth the cost of adding another setting to the pile (compared to combining these into one setting)? See this discussion here:

Adding more controls & optionality is something we would prefer not to do. We would prefer the solution that had the fewest amount of additional controls or settings added because we want to be conscientious of cluttering up the UI (more than it already is, but we're working on that).
As a corollary of 1, that means making development choices that:
a. have good-for-most-everyone default behaviors, and
b. (when 2a does not suffice) lean into our existing settings and controls without having to add additional ones.

@PikachuEXE
Copy link
Collaborator Author

So what's suggested is: replace commentAutoLoadEnabled with this new setting?

@kommunarr
Copy link
Collaborator

I think one could argue for the technical implementation of either replacing the existing setting or adding this as a new behavior of the commentAutoLoadEnabled variable. The latter is maybe less preferable until I finally get an answer on what is actually desired for #4342. But yes, behaviorally, replacing the Comment Auto Load setting with this one.

@PikachuEXE
Copy link
Collaborator Author

Removing commentAutoLoadEnabled is fine for me
As long as we inform users on release note

@PikachuEXE
Copy link
Collaborator Author

Setting commentAutoLoadEnabled removed, tooltip for new setting updated

@FreeTubeBot FreeTubeBot merged commit 2b7c96e into FreeTubeApp:development Apr 16, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kommunarr kommunarr kommunarr approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@absidue absidue absidue approved these changes

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PikachuEXE @kommunarr @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @FreeTubeBot