-
-
Notifications
You must be signed in to change notification settings - Fork 840
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
Create abstract PaginatedListState for DiscussionList and others #2781
Merged
Merged
Changes from all commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
74a1de5
Initial work very WIP not working
dsevillamartin 00b2e95
WIP / Switch DiscussionListState to use PaginatedListState
dsevillamartin b4db89f
Change model type to static prop, add page param to refresh method
dsevillamartin 6bece3c
Move loading states to Promise.finally, rework get next & prev page n…
dsevillamartin 0c664cb
Implement PaginatedListState#loadPrev
dsevillamartin 14eb746
Document params-related methods
dsevillamartin 8ae7c78
Change TYPE into abstract getter, check params.include is array
dsevillamartin 92238f5
Add missing DiscussionListState methods
dsevillamartin f6b5aca
Convert NotificationListState to extend PaginatedListState
dsevillamartin 082f9a4
Create PaginatedListState#getAllItems and #hasItems methods
dsevillamartin bab59c3
Rename 'offset' variable to 'pageSize'
dsevillamartin ab8c7c0
Create protected 'paramsChanged' method for 'refreshParams' extending
dsevillamartin 6a7054e
Reorganize methods, switch to 'loadNext', add 'has' getter methods
dsevillamartin 83b2e91
Mark DiscussionListState methods as deprecated
dsevillamartin 7d01642
Add 'page' param to 'refreshParams'
dsevillamartin 445020a
Return 'this.params' by default in 'requestParams' method
dsevillamartin a0bd5a5
Reset location in NotificationListState#load
dsevillamartin b579ee7
Modify PaginatedListState constructor to accept params, page num, pag…
dsevillamartin d44aaf8
Fix NotificationList component
dsevillamartin 76423a9
Fix DiscussionList loadMore -> loadNext binding
dsevillamartin 17f1996
Use 'isLoading' method for any kind of loading being done
dsevillamartin 622c95d
Reset 'hasPrev' and 'hasNext' values when calling 'clear()'
dsevillamartin 8abe8c7
Add docblock to 'hasItems' and 'isEmpty'
dsevillamartin 29ab58a
Set current location in 'parseResults'
dsevillamartin e646c89
Add 'extraDiscussions' field to DiscussionListState for added discuss…
dsevillamartin 41fb9dc
Fix 'paramsChanged' using 'requestParams' instead of 'getParams()'
dsevillamartin 812e9db
Fix logic error with 'hasNext' and 'hasPrev' using current page and n…
dsevillamartin 75f43a7
Fix math error when page number was a string in 'parseResults'
dsevillamartin d23845b
Remove old JS files of DiscussionListState & NotificationListState
dsevillamartin a9f35b0
Remove <hr> from DiscussionList
dsevillamartin bca2530
Remove useless array spread
dsevillamartin 60e4dc5
Restore clearing paginated lists on refresh
askvortsov1 9965150
Drop deprecated methods
askvortsov1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,230 @@ | ||
import Model from '../Model'; | ||
|
||
export interface Page<TModel> { | ||
number: number; | ||
items: TModel[]; | ||
|
||
hasPrev?: boolean; | ||
hasNext?: boolean; | ||
} | ||
|
||
export interface PaginationLocation { | ||
page: number; | ||
startIndex?: number; | ||
endIndex?: number; | ||
} | ||
|
||
export default abstract class PaginatedListState<T extends Model> { | ||
protected location!: PaginationLocation; | ||
protected pageSize: number; | ||
|
||
protected pages: Page<T>[] = []; | ||
protected params: any = {}; | ||
|
||
protected initialLoading: boolean = false; | ||
protected loadingPrev: boolean = false; | ||
protected loadingNext: boolean = false; | ||
|
||
protected constructor(params: any = {}, page: number = 1, pageSize: number = 20) { | ||
this.params = params; | ||
|
||
this.location = { page }; | ||
this.pageSize = pageSize; | ||
} | ||
|
||
abstract get type(): string; | ||
|
||
public clear() { | ||
this.pages = []; | ||
|
||
m.redraw(); | ||
} | ||
|
||
public loadPrev(): Promise<void> { | ||
if (this.loadingPrev || this.getLocation().page === 1) return Promise.resolve(); | ||
|
||
this.loadingPrev = true; | ||
|
||
const page: number = this.getPrevPageNumber(); | ||
|
||
return this.loadPage(page) | ||
.then(this.parseResults.bind(this, page)) | ||
.finally(() => (this.loadingPrev = false)); | ||
} | ||
|
||
public loadNext(): Promise<void> { | ||
if (this.loadingNext) return Promise.resolve(); | ||
|
||
this.loadingNext = true; | ||
|
||
const page: number = this.getNextPageNumber(); | ||
|
||
return this.loadPage(page) | ||
.then(this.parseResults.bind(this, page)) | ||
.finally(() => (this.loadingNext = false)); | ||
} | ||
|
||
protected parseResults(pg: number, results: T[]) { | ||
const pageNum = Number(pg); | ||
|
||
const links = results.payload?.links || {}; | ||
const page = { | ||
number: pageNum, | ||
items: results, | ||
hasNext: !!links.next, | ||
hasPrev: !!links.prev, | ||
}; | ||
|
||
if (this.isEmpty() || pageNum > this.getNextPageNumber() - 1) { | ||
dsevillamartin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.pages.push(page); | ||
} else { | ||
this.pages.unshift(page); | ||
dsevillamartin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
this.location = { page: pageNum }; | ||
|
||
m.redraw(); | ||
} | ||
|
||
/** | ||
* Load a new page of results. | ||
*/ | ||
protected loadPage(page = 1): Promise<T[]> { | ||
const params = this.requestParams(); | ||
params.page = { offset: this.pageSize * (page - 1) }; | ||
|
||
if (Array.isArray(params.include)) { | ||
params.include = params.include.join(','); | ||
} | ||
|
||
return app.store.find(this.type, params); | ||
} | ||
|
||
/** | ||
* Get the parameters that should be passed in the API request. | ||
* Do not include page offset unless subclass overrides loadPage. | ||
* | ||
* @abstract | ||
* @see loadPage | ||
*/ | ||
protected requestParams(): any { | ||
return this.params; | ||
} | ||
|
||
/** | ||
* Update the `this.params` object, calling `refresh` if they have changed. | ||
* Use `requestParams` for converting `this.params` into API parameters | ||
* | ||
* @param newParams | ||
* @param page | ||
* @see requestParams | ||
*/ | ||
public refreshParams(newParams, page: number) { | ||
if (this.isEmpty() || this.paramsChanged(newParams)) { | ||
this.params = newParams; | ||
|
||
return this.refresh(page); | ||
} | ||
} | ||
|
||
public refresh(page: number = 1) { | ||
this.initialLoading = true; | ||
this.loadingPrev = false; | ||
this.loadingNext = false; | ||
|
||
this.clear(); | ||
|
||
this.location = { page }; | ||
|
||
return this.loadPage() | ||
.then((results: T[]) => { | ||
this.pages = []; | ||
this.parseResults(this.location.page, results); | ||
}) | ||
.finally(() => (this.initialLoading = false)); | ||
} | ||
|
||
public getPages() { | ||
return this.pages; | ||
} | ||
public getLocation(): PaginationLocation { | ||
return this.location; | ||
} | ||
|
||
public isLoading(): boolean { | ||
return this.initialLoading || this.loadingNext || this.loadingPrev; | ||
} | ||
public isInitialLoading(): boolean { | ||
return this.initialLoading; | ||
} | ||
public isLoadingPrev(): boolean { | ||
return this.loadingPrev; | ||
} | ||
public isLoadingNext(): boolean { | ||
return this.loadingNext; | ||
} | ||
|
||
/** | ||
* Returns true when the number of items across all loaded pages is not 0. | ||
* | ||
* @see isEmpty | ||
*/ | ||
public hasItems(): boolean { | ||
return !!this.getAllItems().length; | ||
} | ||
|
||
/** | ||
* Returns true when there aren't any items *and* the state has already done its initial loading. | ||
* If you want to know whether there are items regardless of load state, use `hasItems()` instead | ||
* | ||
* @see hasItems | ||
*/ | ||
public isEmpty(): boolean { | ||
return !this.isInitialLoading() && !this.hasItems(); | ||
} | ||
|
||
public hasPrev(): boolean { | ||
return !!this.pages[0]?.hasPrev; | ||
} | ||
public hasNext(): boolean { | ||
return !!this.pages[this.pages.length - 1]?.hasNext; | ||
} | ||
|
||
/** | ||
* Stored state parameters. | ||
*/ | ||
public getParams(): any { | ||
return this.params; | ||
} | ||
|
||
protected getNextPageNumber(): number { | ||
const pg = this.pages[this.pages.length - 1]?.number; | ||
|
||
if (pg && !isNaN(pg)) { | ||
return pg + 1; | ||
} else { | ||
return this.location.page; | ||
} | ||
} | ||
protected getPrevPageNumber(): number { | ||
const pg = this.pages[0]?.number; | ||
|
||
if (pg && !isNaN(pg)) { | ||
// If the calculated page number is less than 1, | ||
// return 1 as the prev page (first possible page number) | ||
return Math.max(pg - 1, 1); | ||
} else { | ||
return this.location.page; | ||
} | ||
} | ||
|
||
protected paramsChanged(newParams): boolean { | ||
return Object.keys(newParams).some((key) => this.getParams()[key] !== newParams[key]); | ||
} | ||
|
||
protected getAllItems(): T[] { | ||
return this.getPages() | ||
.map((pg) => pg.items) | ||
.flat(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
'spage
prop should point to the active element. If it's an array,location
'spage
prop should be the index of the active page (although we'd need to remember to increment by 1 when adding new pages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the previous page be the page that the user is currently on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Active page, sorry.