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(pagination): smart pagination #479

Merged
merged 11 commits into from
Mar 10, 2023
Merged

feat(pagination): smart pagination #479

merged 11 commits into from
Mar 10, 2023

Conversation

bassrock
Copy link
Member

@bassrock bassrock commented Mar 10, 2023

Summary

  • Ensure we smartly paginate our results. The first page will be smaller so the user sees content and the next page will be larger. We also should only load in the first 500 items in a users list for now.

References

Implementation Details

  • Chose 500 items for now, will align with @nzeltzer and leadership later on for our launch size.

  • I also updated our large lists to have new data, but didn't change the data within. I want to find a tool to do that for us instead of sitting manually doing that.

Test Steps

  • Load your list, look at logs and confirm first page is 15 items, subsequent is 30.

PR Checklist:

  • Added Unit / UI tests
  • Self Review (review, clean up, documentation, run tests)
  • Basic Self QA

Screenshots

@bassrock bassrock marked this pull request as ready for review March 10, 2023 16:45
@bassrock bassrock requested a review from a team as a code owner March 10, 2023 16:45
@bassrock bassrock changed the title Smart pagination feat(pagination): smart pagination Mar 10, 2023
@pocket-ci
Copy link
Contributor

pocket-ci commented Mar 10, 2023

Messages
📖 No SwiftLint violations! 🎉
📖 Project coverage: 77.36%
📖 Edited 22 files
📖 Created 1 files

PocketKit: Coverage: 75.59

File Coverage
RefreshCoordinator.swift 100.0%
RootViewModel.swift 96.15%

Sync: Coverage: 88.96

File Coverage
PocketSource.swift 86.56%
FetchArchive.swift 87.6%
SyncTask.swift 88.89%
SyncOperationFactory.swift 100.0%
Source+async.swift 0.0% ⚠️
SlateService.swift 81.48%
FetchSaves.swift 98.77%
Source.swift 100.0%

Generated by 🚫 Danger Swift against 33faed6

Comment on lines 1 to 6
//
// File.swift
//
//
// Created by Daniel Brooks on 3/9/23.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

update with other Source Code Form header?


import Foundation

struct SyncConstants {
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! maybe we want to add Search constants here too in the future

@@ -199,6 +198,9 @@ class FetchSavesTests: XCTestCase {

func test_refresh_whenItemCountExceedsMax_fetchesMaxNumberOfItems() async throws {
var fetches = 0
let pages = Int(ceil(Double((SyncConstants.Saves.firstLoadMaxCount - SyncConstants.Saves.initalPageSize) / SyncConstants.Saves.pageSize))) + 2
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we add tests for Archive as well?

Copy link
Collaborator

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Looks good @bassrock , the log reported what you say in the description!

Copy link
Contributor

@cyndichin cyndichin left a comment

Choose a reason for hiding this comment

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

logs look good, I see initial 15 and subsequent 30. a couple of comments, but non blocker!

@bassrock bassrock merged commit 14c0d02 into develop Mar 10, 2023
@bassrock bassrock deleted the smart-pagination branch March 10, 2023 23:32
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.

4 participants