Skip to content

[Feature] Cursor based pagination [#OSF-8565]#7715

Open
mfraezz wants to merge 2 commits intoCenterForOpenScience:developfrom
mfraezz:feature/cursor-based-pagination
Open

[Feature] Cursor based pagination [#OSF-8565]#7715
mfraezz wants to merge 2 commits intoCenterForOpenScience:developfrom
mfraezz:feature/cursor-based-pagination

Conversation

@mfraezz
Copy link
Copy Markdown
Member

@mfraezz mfraezz commented Sep 15, 2017

Purpose

Improve NodeLogList performance

Changes

  • Create JSONAPI-compliant subclass for rest_framework.pagination.CursorPagination
  • Use on NodeLogList
  • Update frontend

Side effects

None expected

Ticket

[OSF-8565]

@mfraezz mfraezz force-pushed the feature/cursor-based-pagination branch from 396ea80 to 628eea2 Compare September 15, 2017 20:03
@sloria
Copy link
Copy Markdown
Contributor

sloria commented Sep 25, 2017

This is a pretty significant, backwards-incompatible change (worthy of more than a minor version bump). I wonder if we should keep the current pagination scheme to preserve compat but allow changing the pagination style via a query parameter. Looks like there's even a package for that.

Thoughts, @brianjgeiger ?

@sloria
Copy link
Copy Markdown
Contributor

sloria commented Sep 25, 2017

The loading indicator for the log feed got lost with this change.

That said, the log feed was using the now-deprecated OSF loading spinner. Two options:

  • Good: Update the log feed to use a new spinner (.ball-pulse).
  • Better: Use a skeleton screen as a loading indicator. A quick-and-dirty way to achieve this is to reduce the opacity of the items when the widget is in the loading state. It's not exactly the same as a skeleton, but at least it doesn't change the height of the widget as much:

osf___small_project

sloria
sloria previously requested changes Sep 25, 2017
Copy link
Copy Markdown
Contributor

@sloria sloria left a comment

Choose a reason for hiding this comment

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

Pass finished for now. See if you can fix the loading indicator for now. We'll discuss whether or not to keep backwards compat for the paging style.

@mfraezz
Copy link
Copy Markdown
Member Author

mfraezz commented Sep 25, 2017

According to discussion with @brianjgeiger sometime over the past two weeks, this does not merit an API version bump. The values for linked references change, yes, but the rationale is that the client should only be following the resp.json['links'], the references for which do not change. That is, client-side url construction is not a supported use case; they should be using links provided in the response.

I'll fix the loading UI

@sloria
Copy link
Copy Markdown
Contributor

sloria commented Sep 25, 2017

@mfraezz Ah ok, thanks for the clarification. I wasn't sure if the keys of the pagination object were changing here or not. If not, then I agree: no need to bump versions.

@mfraezz mfraezz force-pushed the feature/cursor-based-pagination branch 2 times, most recently from 7644511 to 757ff51 Compare September 25, 2017 19:52
@mfraezz mfraezz dismissed sloria’s stale review October 10, 2017 20:01

Requested changes addressed

@mfraezz mfraezz force-pushed the feature/cursor-based-pagination branch from 757ff51 to 17c561d Compare November 20, 2017 20:06
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.

2 participants