Create abstract PaginatedListState for DiscussionList and others #2781

dsevillamartin · 2021-04-11T21:49:30Z

Refs flarum/issue-archive#162

Reviewers should focus on:

Look at // TODO comments in code

What does the current page track?
Do we want the abstract state tied to store models?
How do we want to treat this.pages?
Logic to set when loading/done loading needs to be revised
Calling refresh sets page location to pg 1, this needs to be changed (refresh is called on page load, will replace params or whatever)
The params implementation in general needs clarification - do we want like DiscussionListState or something else?

Confirmed

Frontend changes: tested on a local Flarum installation.

Required changes:

Related documentation PR: (Remove if irrelevant)

askvortsov1

The params implementation in general needs clarification - do we want like DiscussionListState or something else?

I think DiscussionListState's implementation makes sense, with one exception: in FriendsOfFlarum/user-directory@bc4ffe8, Ian and I added a "qBuilder", which makes it easier to assemble the q query string. I think that's something we might want to move over into the official implementation.

On a side note, to keep things simple, we might want to do this in 2 phases:

Swap out DiscussionListState for PaginatedListState, implement the url pagination
Possibly in a separate release (since Improve discussion page near canonical url #2397 is still on the milestone), swap out DiscussionList for an abstract PaginatedList

askvortsov1 · 2021-04-13T02:16:31Z

js/src/common/states/PaginatedListState.ts

+  //  - what does this page track? last page request? last page that is loaded
+  //      (e.g. pg 7 if u have 3-7 loaded)
+  //  - When do we set it?
+  protected location!: PaginationLocation;


pages is a contiguous structure that expands from both ends. So it could either be an array or a doubly linked list. If it's a doubly linked list, location's page prop should point to the active element. If it's an array, location's page prop should be the index of the active page (although we'd need to remember to increment by 1 when adding new pages.

What are we defining with "active" page though? If the user loads the previous page, does the active page still remain the last number in ascending order (e.g. loaded pg 2 but pg 5 is still active) or does it change to page 2?

Shouldn't the previous page be the page that the user is currently on?

What are you referring to with previous page? I meant as load next page, load prev page, for loading a page that hasn't been loaded.

Active page, sorry.

js/src/common/states/PaginatedListState.ts

dsevillamartin · 2021-04-22T00:28:01Z

Regarding the handling of query parameters, some thoughts from Sasha:

A default params method is good
As is a method to assemble params before sending (since we'd like to add in a qBuilder field)
e.g. requestParams in DiscussionListState.js
So the params object will be in a "construction" form, not fully in the format it'd be sent in, for some of the methods and for internal storage

askvortsov1 · 2021-04-22T19:31:33Z

Looked at this again. DiscussionListState only has 2 methods: one for getting the underlying params object, the other for converting that into the format expected by JSON:API. I think having these 2 methods is sensible for PaginatedList. It shouldn't need to worry about global / URL query params IMO.

Have you tried plugging this into the NotificationList? Does it work as well as with DiscussionList?

dsevillamartin · 2021-04-25T13:38:24Z

Have you tried plugging this into the NotificationList?

I have not. I will try to do so today.

dsevillamartin · 2021-04-25T21:25:47Z

How do we feel about the getAllItems method in c7d129b?
How do we like the DiscussionListState#removeDiscussion implementation in e01ab57?
How do we want to handle DiscussionListState#addDiscussion
- Do we want an extra field for the added discussions that is cleared on refresh?
  That way we don't do weird stuff with pagination.
  We can have an extra method that adds the extra field to the array of discussions returned.
Do we want getter methods for hasPrev, hasNext? (I assume yes)
Do we want to keep methods like hasDiscussions and empty that call to the actual methods for BC?

askvortsov1

Do we want getter methods for hasPrev, hasNext? (I assume yes)

I think so, if only for the consistency with other methods

Do we want to keep methods like hasDiscussions and empty that call to the actual methods for BC?

Yes, but let's deprecate them. Before the release, we can do a sweep and decide which deprecated stuff to drop (since the next time we'll get the chance to do so will be v2)

Do we want an extra field for the added discussions that is cleared on refresh?

That way we don't do weird stuff with pagination.
We can have an extra method that adds the extra field to the array of discussions returned.

So when the next page comes in, we'd check if the added discussion is present, and if so, override it in the returned results? I like this idea.

How do we feel about the getAllItems method in c7d129b?

Makes sense to me. Let's keep it protected for now though.

How do we like the DiscussionListState#removeDiscussion implementation in e01ab57?

Also looks good to me!

I'm not seeing any code for navigating to the previous page in the component. Will this be part of the followup component PR?

js/src/common/states/PaginatedListState.ts

js/src/forum/components/DiscussionList.js

js/src/forum/components/IndexPage.js

js/src/common/states/PaginatedListState.ts

js/src/forum/components/IndexPage.js

js/src/forum/states/DiscussionListState.ts

askvortsov1 · 2021-04-26T18:43:58Z

js/src/forum/states/NotificationListState.ts

+   */
+  load(): Promise<void> {
+    if (app.session.user.newNotificationCount()) {
+      this.pages = [];


Shouldn't this reset location too?

Do we want a method to reset location perhaps? Not sure. Don't like having this.location = { page: 1 } a lot tbh.

js/src/common/states/PaginatedListState.ts

askvortsov1

I've given it some more thought, and I'm no longer sure that qBuilder should go here. I think it might make sense to put it in GlobalSearchState instead (which is a refactor for another day).

askvortsov1 · 2021-04-30T01:44:31Z

js/src/common/states/PaginatedListState.ts

+    this.loadingPrev = false;
+    this.loadingNext = false;
+
+    m.redraw();


We currently clear it in DiscussionList here so that the loading indicator is shown while refreshing, IIRC

js/src/common/states/PaginatedListState.ts

dsevillamartin · 2021-05-09T13:16:39Z

js/src/forum/states/DiscussionListState.ts

+    if (this.extraDiscussions.length) {
+      return [
+        {
+          number: -1,
+          items: this.extraDiscussions,
+        },
+        ...pages,
+      ];
+    }


I don't love this, but I don't know how else we could do this.

This is so that DiscussionList can do .getPages() and add a page separator (as an example) that forum owners can then style.

Not sure whether the state or the component should check whether the page is empty, it's currently on the state (checks that extraDiscussions isn't empty before adding the page).

dsevillamartin · 2021-05-09T13:41:23Z

I found a logic error - hasNext and hasPrev are calculated using the current page, and not the earliest or latest pages loaded.

So, if there are 3 pages of discussions, and we begin by loading the 3rd, then load the 2nd, the "load more" button appears because there are more after the 2nd page, even though the 3rd is already loaded.

I think the fix for this would be to move the hasPrev and hasNext values to the Page interface, and then use the methods to check the first & last items in this.pages. Thoughts?

askvortsov1 · 2021-05-09T17:16:50Z

I found a logic error - hasNext and hasPrev are calculated using the current page, and not the earliest or latest pages loaded.

So, if there are 3 pages of discussions, and we begin by loading the 3rd, then load the 2nd, the "load more" button appears because there are more after the 2nd page, even though the 3rd is already loaded.

I think the fix for this would be to move the hasPrev and hasNext values to the Page interface, and then use the methods to check the first & last items in this.pages. Thoughts?

That makes sense to me!

…umber code

…e size

…ions

…ot considering loaded pages

dsevillamartin · 2021-05-10T19:35:16Z

This is ready to review now. I'll try moving the unnecessary UI changes for DiscussionList to another PR so that can be discussed there (e.g. the <hr> marking page & load prev button).

After I do that I'll mark as ready for review.

This is needed for visual confirmation with a loading indicator

askvortsov1 reviewed Apr 13, 2021

View reviewed changes

askvortsov1 requested changes Apr 26, 2021

View reviewed changes

askvortsov1 requested changes May 2, 2021

View reviewed changes

dsevillamartin force-pushed the ds/create-pagination-utility branch from 5030e1b to e5538b7 Compare May 9, 2021 01:44

dsevillamartin commented May 9, 2021

View reviewed changes

askvortsov1 added this to the 1.0 milestone May 9, 2021

dsevillamartin added 18 commits May 10, 2021 15:27

Initial work very WIP not working

74a1de5

WIP / Switch DiscussionListState to use PaginatedListState

00b2e95

Change model type to static prop, add page param to refresh method

b4db89f

Move loading states to Promise.finally, rework get next & prev page n…

6bece3c

…umber code

Implement PaginatedListState#loadPrev

0c664cb

Document params-related methods

14eb746

Change TYPE into abstract getter, check params.include is array

8ae7c78

Add missing DiscussionListState methods

92238f5

Convert NotificationListState to extend PaginatedListState

f6b5aca

Create PaginatedListState#getAllItems and #hasItems methods

082f9a4

Rename 'offset' variable to 'pageSize'

bab59c3

Create protected 'paramsChanged' method for 'refreshParams' extending

ab8c7c0

Reorganize methods, switch to 'loadNext', add 'has' getter methods

6a7054e

Mark DiscussionListState methods as deprecated

83b2e91

Add 'page' param to 'refreshParams'

7d01642

Return 'this.params' by default in 'requestParams' method

445020a

Reset location in NotificationListState#load

a0bd5a5

Modify PaginatedListState constructor to accept params, page num, pag…

b579ee7

…e size

dsevillamartin added 11 commits May 10, 2021 15:31

Fix NotificationList component

d44aaf8

Fix DiscussionList loadMore -> loadNext binding

76423a9

Use 'isLoading' method for any kind of loading being done

17f1996

Reset 'hasPrev' and 'hasNext' values when calling 'clear()'

622c95d

Add docblock to 'hasItems' and 'isEmpty'

8abe8c7

Set current location in 'parseResults'

29ab58a

Add 'extraDiscussions' field to DiscussionListState for added discuss…

e646c89

…ions

Fix 'paramsChanged' using 'requestParams' instead of 'getParams()'

41fb9dc

Fix logic error with 'hasNext' and 'hasPrev' using current page and n…

812e9db

…ot considering loaded pages

Fix math error when page number was a string in 'parseResults'

75f43a7

Remove old JS files of DiscussionListState & NotificationListState

d23845b

dsevillamartin force-pushed the ds/create-pagination-utility branch from 5b54b2a to d23845b Compare May 10, 2021 19:32

dsevillamartin added 2 commits May 10, 2021 16:45

Remove <hr> from DiscussionList

a9f35b0

Remove useless array spread

bca2530

dsevillamartin marked this pull request as ready for review May 10, 2021 20:49

dsevillamartin changed the title ~~[WIP] Create abstract PaginatedListState for DiscussionList and others~~ Create abstract PaginatedListState for DiscussionList and others May 10, 2021

dsevillamartin mentioned this pull request May 10, 2021

Add page separators and load previous button to DiscussionList #2851

Closed

2 tasks

askvortsov1 added 2 commits May 10, 2021 18:12

Restore clearing paginated lists on refresh

60e4dc5

This is needed for visual confirmation with a loading indicator

Drop deprecated methods

9965150

askvortsov1 approved these changes May 10, 2021

View reviewed changes

askvortsov1 requested review from davwheat and SychO9 May 10, 2021 22:17

SychO9 approved these changes May 11, 2021

View reviewed changes

askvortsov1 merged commit 4e0fdb4 into master May 11, 2021

askvortsov1 deleted the ds/create-pagination-utility branch May 11, 2021 23:14

dsevillamartin mentioned this pull request May 6, 2021

Notifications panel scrolling doesn't work on high-resolution displays flarum/issue-archive#120

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create abstract PaginatedListState for DiscussionList and others #2781

Create abstract PaginatedListState for DiscussionList and others #2781

dsevillamartin commented Apr 11, 2021 •

edited

Loading

askvortsov1 left a comment •

edited

Loading

askvortsov1 Apr 13, 2021

dsevillamartin Apr 13, 2021

askvortsov1 Apr 13, 2021 •

edited

Loading

dsevillamartin Apr 13, 2021

askvortsov1 Apr 13, 2021

dsevillamartin commented Apr 22, 2021

askvortsov1 commented Apr 22, 2021

dsevillamartin commented Apr 25, 2021

dsevillamartin commented Apr 25, 2021 •

edited

Loading

askvortsov1 left a comment •

edited

Loading

askvortsov1 Apr 26, 2021

dsevillamartin Apr 29, 2021

askvortsov1 left a comment

askvortsov1 Apr 30, 2021

dsevillamartin May 9, 2021

dsevillamartin commented May 9, 2021

askvortsov1 commented May 9, 2021

dsevillamartin commented May 10, 2021 •

edited

Loading

Create abstract PaginatedListState for DiscussionList and others #2781

Create abstract PaginatedListState for DiscussionList and others #2781

Conversation

dsevillamartin commented Apr 11, 2021 • edited Loading

askvortsov1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsevillamartin commented Apr 22, 2021

askvortsov1 commented Apr 22, 2021

dsevillamartin commented Apr 25, 2021

dsevillamartin commented Apr 25, 2021 • edited Loading

askvortsov1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askvortsov1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsevillamartin commented May 9, 2021

askvortsov1 commented May 9, 2021

dsevillamartin commented May 10, 2021 • edited Loading

dsevillamartin commented Apr 11, 2021 •

edited

Loading

askvortsov1 left a comment •

edited

Loading

askvortsov1 Apr 13, 2021 •

edited

Loading

dsevillamartin commented Apr 25, 2021 •

edited

Loading

askvortsov1 left a comment •

edited

Loading

dsevillamartin commented May 10, 2021 •

edited

Loading