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

Infinite scroll SEO #6598

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

Conversation

kontrollanten
Copy link
Contributor

@kontrollanten kontrollanten commented Sep 20, 2024

Description

Adds a "Load more" button on the bottom of MyInfiniteScrollerComponent. For a regular user the behavior should be the same.

Behavior for bots:

  1. Loads the page and InifniteScroller will continue to load more content until the visible part of the page is full.
  2. In the bottom the bot will see a link to page?{currentPage++}
  3. Upon visiting this link the scenario will be repeated.

Background

Thoughts during the path

  • Skip the scroll logic and instead use IntersectionObserver to trigger "near bottom" when the Load more button becomes visible.
  • I began to add pagination URL query params upon scroll, but this went too complex. Maybe it can be nice for users to be able to link directly to a paginated page, but it's out of scope for this feature.
  • When watching a video in a playlist the query params will affect both the playlist and comments. Not ideal, but will only affect bots.

Todo

  • Add page query param to the canonical URL for pages where the main content is paginated. For other paginated pages (i.e. comments, watch playlist, etc) the canonical URL shouldn't include the page query param.

Related issues

#6332

Has this been tested?

  • 👍 yes, I added tests to the test suite
  • 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this PR does not update server code
  • 🙋 no, because I need help

To test it manually, add a "?page=1" query param to a route which has infinite scroll

Screenshots

Add a "Load more" button in the bottom to help search engine bots to
navigate to the next page. In order to debug this functionality, add
?finiteScroll=true to the URL.
hasMoreItems has to be called before bumping the current page in order
to work accordingly.
@kontrollanten kontrollanten marked this pull request as draft September 20, 2024 19:04
@kontrollanten kontrollanten marked this pull request as ready for review September 24, 2024 10:44
if (!hasMoreItems(this.pagination)) return
if (!hasMoreItems(this.pagination)) {
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

if (this.pagination.totalItems <= (this.pagination.currentPage * this.pagination.itemsPerPage)) return
if (this.pagination.totalItems <= (this.pagination.currentPage * this.pagination.itemsPerPage)) {
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

search: string

constructor (
private userSubscriptionService: UserSubscriptionService,
private notifier: Notifier
) {}
) { }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

if (this.pagination.totalItems <= (this.pagination.currentPage * this.pagination.itemsPerPage)) return
if (this.pagination.totalItems <= (this.pagination.currentPage * this.pagination.itemsPerPage)) {
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

this.pagination.totalItems = null
this.pagination.currentPage = 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

@@ -152,7 +153,8 @@ export class VideosListComponent implements OnInit, OnChanges, OnDestroy {
private route: ActivatedRoute,
private screenService: ScreenService,
private peertubeRouter: PeerTubeRouterService,
private serverService: ServerService
private serverService: ServerService,
public router: Router
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used?

if (this.lastQueryLength !== undefined && this.lastQueryLength < this.pagination.itemsPerPage) return
if (this.lastQueryLength !== undefined && this.lastQueryLength < this.pagination.itemsPerPage) {
return
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

if (hasMoreItems(this.componentPagination)) {
this.loadNotifications()
}

this.componentPagination.currentPage++
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 should be at line 89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove

@@ -15,7 +15,6 @@ import { PageHtml } from './page-html.js'
import { TagsHtml } from './tags-html.js'

export class VideoHtml {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert

@kontrollanten
Copy link
Contributor Author

@Chocobozzz I did a self-review and found some minor style changes. If you're okay with this implementation I'll fix them.

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.

1 participant