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

feat: people infinite scroll #11326

Merged
merged 5 commits into from
Jul 25, 2024
Merged

feat: people infinite scroll #11326

merged 5 commits into from
Jul 25, 2024

Conversation

michelheusschen
Copy link
Contributor

Adds pagination for people to the server and infinite scroll to the people page on web for viewing more then 500 people.

  • Order by person.createdAt to have results in the same order every time
  • hasNextPage is optional to keep backwards compatibility with the mobile app
  • Web will see some performance issues when loading a lot of people. I plan to address this in a future PR using virtual scroll.

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

With the pagination not being optional this is a breaking change, correct? Besides, that, the code looks really good! You're on a rollllll

@IsInt()
@Min(1)
@Max(1000)
@Type(() => Number)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the type annotation, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The conversion is necessary because implicit conversions are disabled. Pagination is optional with the same defaults as before, so there shouldn't be any breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok cool!

Copy link
Contributor

Choose a reason for hiding this comment

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

We only do because it is a query param so it would be a string otherwise.

@bo0tzz
Copy link
Member

bo0tzz commented Jul 24, 2024

Order by person.createdAt to have results in the same order every time

This means the person page is no longer ordered by how many photos there are of a person, right?

@michelheusschen
Copy link
Contributor Author

No, the current order is kept. If you have multiple unnamed people with the same photo count the order would previously be undefined which is a problem for pagination. Now it additionally sorts by person.createdAt to have a consistent sort order.

@michelheusschen michelheusschen linked an issue Jul 24, 2024 that may be closed by this pull request
3 tasks
server/src/dtos/person.dto.ts Outdated Show resolved Hide resolved
@jrasm91 jrasm91 merged commit 8e6bc13 into main Jul 25, 2024
22 checks passed
@jrasm91 jrasm91 deleted the feat/people-infinite-scroll branch July 25, 2024 19:59
@mertalev
Copy link
Contributor

The only thing with this is that it will scan and sort all the faces in the table to count them, then do that again for the next page. But that’s still much better than not being able to see the next page at all.

@mmomjian mmomjian mentioned this pull request Oct 17, 2024
3 tasks
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.

Number of people shown is limited to 500 (after update to v1.108.0)
5 participants