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: add p/avl/pager #2584

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

feat: add p/avl/pager #2584

wants to merge 21 commits into from

Conversation

moul
Copy link
Member

@moul moul commented Jul 14, 2024

  • add p/demo/avl/pager
  • update r/demo/users

Hey reviewers, in addition to what you wanted to review, I'm specifically curious if you have any better API/usage ideas.

Example: https://github.com/gnolang/gno/pull/2584/files#diff-8d5cbbe072737a7f288f74adcaaace11cacc3d31264e6a001515fcae824394e2R33

Related with #447, #599, #868

@moul moul self-assigned this Jul 14, 2024
@moul moul changed the title dev/moul/pagination feat: add p/avl/pager Jul 14, 2024
@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Jul 14, 2024
@moul moul marked this pull request as ready for review July 14, 2024 11:44
@moul moul requested review from a team and jaekwon as code owners July 14, 2024 11:44
@moul moul requested review from ajnavarro and removed request for a team July 14, 2024 11:44
Signed-off-by: moul <[email protected]>
@moul moul mentioned this pull request Jul 14, 2024
7 tasks
Signed-off-by: moul <[email protected]>
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.95%. Comparing base (8a62a28) to head (943df0c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2584      +/-   ##
==========================================
- Coverage   60.95%   60.95%   -0.01%     
==========================================
  Files         564      564              
  Lines       75273    75273              
==========================================
- Hits        45884    45879       -5     
- Misses      26019    26022       +3     
- Partials     3370     3372       +2     
Flag Coverage Δ
contribs/gnodev 61.46% <ø> (ø)
contribs/gnofaucet 14.46% <ø> (ø)
gno.land 67.92% <ø> (ø)
gnovm 65.77% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 19.88% <ø> (ø)
tm2 62.05% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: moul <[email protected]>
@moul moul marked this pull request as ready for review September 24, 2024 20:29
@moul
Copy link
Member Author

moul commented Sep 30, 2024

@gnolang/devx @gnolang/berty, since we will be using Pager across all social dApps, please review this PR with a focus on its usage and API. Thank you.

(Open to anyone, by the way.)

Copy link
Contributor

@wyhaines wyhaines left a comment

Choose a reason for hiding this comment

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

This is clean and easy for me to follow. From my POV, I think that this looks good.

@jefft0
Copy link
Contributor

jefft0 commented Oct 1, 2024

I tried the pagination with the following:

git clone https://github.com/moul/gno --branch dev/moul/pagination
cd gno
make install

Edit examples/gno.land/r/demo/users/users.gno. On line 331 in NewPager(&name2User, 50), change 50 to 5.

gnodev

In a browser, go to http://127.0.0.1:8888/r/demo/users . It shows:

    archives
    demo
    gno
    gnoland
    gnolang 1 | 2

When I click "2", it doesn't change.

return doc
}

func splitPathAndQuery(fullPath string) (string, string) {
parts := strings.SplitN(fullPath, "?", 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

When I click on page "2", the URL becomes "http://127.0.0.1:8888/r/demo/users?page=2". But I don't think that the query string with the "?" is passed to Render.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Created #2876 in case is helpful

Comment on lines +57 to +94
if pageSize < 1 {
return &Page{
Items: []Item{},
PageNumber: 0,
PageSize: 0,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: false,
HasNext: false,
Pager: p,
}
}

if pageNumber < 1 {
return &Page{
Items: []Item{},
PageNumber: 0,
PageSize: pageSize,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: false,
HasNext: pageNumber < totalPages,
Pager: p,
}
}

if pageNumber > totalPages {
return &Page{
Items: []Item{},
PageNumber: pageNumber,
PageSize: pageSize,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: totalPages > 0,
HasNext: false,
Pager: p,
}
}
Copy link
Contributor

@ajnavarro ajnavarro Oct 3, 2024

Choose a reason for hiding this comment

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

Suggested change
if pageSize < 1 {
return &Page{
Items: []Item{},
PageNumber: 0,
PageSize: 0,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: false,
HasNext: false,
Pager: p,
}
}
if pageNumber < 1 {
return &Page{
Items: []Item{},
PageNumber: 0,
PageSize: pageSize,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: false,
HasNext: pageNumber < totalPages,
Pager: p,
}
}
if pageNumber > totalPages {
return &Page{
Items: []Item{},
PageNumber: pageNumber,
PageSize: pageSize,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: totalPages > 0,
HasNext: false,
Pager: p,
}
}
page := &Page{
TotalItems: totalItems,
TotalPages: totalPages,
PageSize: pageSize,
Pager: p,
}
// pages without content
if pageSize < 1 {
return page
}
// page number provided is not available
if pageNumber < 1 {
page.HasNext = pageNumber < totalPages
return page
}
// page number provided is outside the range of total pages
if pageNumber > totalPages {
page.PageNumber = pageNumber
page.HasPrev = totalPages > 0
return page
}

items := []Item{}
p.Tree.ReverseIterateByOffset(startIndex, endIndex-startIndex, func(key string, value interface{}) bool {
items = append(items, Item{Key: key, Value: value})
return false
Copy link
Contributor

@ajnavarro ajnavarro Oct 3, 2024

Choose a reason for hiding this comment

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

offtopic: a bit misleading. Normally, if you want to continue, you return true, but it is what it is. Example from standard library: https://pkg.go.dev/sync#Map.Range

Comment on lines +108 to +117
return &Page{
Items: items,
PageNumber: pageNumber,
PageSize: pageSize,
TotalItems: totalItems,
TotalPages: totalPages,
HasPrev: pageNumber > 1,
HasNext: pageNumber < totalPages,
Pager: p,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

here, you can add items to previously created page instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: In Progress
Status: 📥 Inbox
Status: Agenda
Status: In Review
Development

Successfully merging this pull request may close these issues.

5 participants