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

Create abstract PaginatedListState for DiscussionList and others #2781

Merged
merged 33 commits into from
May 11, 2021
Merged
Show file tree
Hide file tree
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 Mar 27, 2021
00b2e95
WIP / Switch DiscussionListState to use PaginatedListState
dsevillamartin Apr 11, 2021
b4db89f
Change model type to static prop, add page param to refresh method
dsevillamartin Apr 22, 2021
6bece3c
Move loading states to Promise.finally, rework get next & prev page n…
dsevillamartin Apr 25, 2021
0c664cb
Implement PaginatedListState#loadPrev
dsevillamartin Apr 25, 2021
14eb746
Document params-related methods
dsevillamartin Apr 25, 2021
8ae7c78
Change TYPE into abstract getter, check params.include is array
dsevillamartin Apr 25, 2021
92238f5
Add missing DiscussionListState methods
dsevillamartin Apr 25, 2021
f6b5aca
Convert NotificationListState to extend PaginatedListState
dsevillamartin Apr 25, 2021
082f9a4
Create PaginatedListState#getAllItems and #hasItems methods
dsevillamartin Apr 25, 2021
bab59c3
Rename 'offset' variable to 'pageSize'
dsevillamartin Apr 29, 2021
ab8c7c0
Create protected 'paramsChanged' method for 'refreshParams' extending
dsevillamartin Apr 29, 2021
6a7054e
Reorganize methods, switch to 'loadNext', add 'has' getter methods
dsevillamartin Apr 29, 2021
83b2e91
Mark DiscussionListState methods as deprecated
dsevillamartin Apr 29, 2021
7d01642
Add 'page' param to 'refreshParams'
dsevillamartin Apr 29, 2021
445020a
Return 'this.params' by default in 'requestParams' method
dsevillamartin Apr 29, 2021
a0bd5a5
Reset location in NotificationListState#load
dsevillamartin Apr 29, 2021
b579ee7
Modify PaginatedListState constructor to accept params, page num, pag…
dsevillamartin Apr 30, 2021
d44aaf8
Fix NotificationList component
dsevillamartin May 9, 2021
76423a9
Fix DiscussionList loadMore -> loadNext binding
dsevillamartin May 9, 2021
17f1996
Use 'isLoading' method for any kind of loading being done
dsevillamartin May 9, 2021
622c95d
Reset 'hasPrev' and 'hasNext' values when calling 'clear()'
dsevillamartin May 9, 2021
8abe8c7
Add docblock to 'hasItems' and 'isEmpty'
dsevillamartin May 9, 2021
29ab58a
Set current location in 'parseResults'
dsevillamartin May 9, 2021
e646c89
Add 'extraDiscussions' field to DiscussionListState for added discuss…
dsevillamartin May 9, 2021
41fb9dc
Fix 'paramsChanged' using 'requestParams' instead of 'getParams()'
dsevillamartin May 10, 2021
812e9db
Fix logic error with 'hasNext' and 'hasPrev' using current page and n…
dsevillamartin May 10, 2021
75f43a7
Fix math error when page number was a string in 'parseResults'
dsevillamartin May 10, 2021
d23845b
Remove old JS files of DiscussionListState & NotificationListState
dsevillamartin May 10, 2021
a9f35b0
Remove <hr> from DiscussionList
dsevillamartin May 10, 2021
bca2530
Remove useless array spread
dsevillamartin May 10, 2021
60e4dc5
Restore clearing paginated lists on refresh
askvortsov1 May 10, 2021
9965150
Drop deprecated methods
askvortsov1 May 10, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions js/src/common/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import userOnline from './helpers/userOnline';
import listItems from './helpers/listItems';
import Fragment from './Fragment';
import DefaultResolver from './resolvers/DefaultResolver';
import PaginatedListState from './states/PaginatedListState';

export default {
extend: extend,
Expand Down Expand Up @@ -154,4 +155,5 @@ export default {
'helpers/userOnline': userOnline,
'helpers/listItems': listItems,
'resolvers/DefaultResolver': DefaultResolver,
'states/PaginatedListState': PaginatedListState,
};
230 changes: 230 additions & 0 deletions js/src/common/states/PaginatedListState.ts
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;
Copy link
Sponsor Member

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'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.

Copy link
Member Author

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?

Copy link
Sponsor Member

@askvortsov1 askvortsov1 Apr 13, 2021

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?

Copy link
Member Author

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Active page, sorry.

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();
}
}
2 changes: 1 addition & 1 deletion js/src/forum/ForumApplication.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export default class ForumApplication extends Application {
*
* @type {DiscussionListState}
*/
this.discussions = new DiscussionListState({}, this);
this.discussions = new DiscussionListState({});
}

/**
Expand Down
18 changes: 11 additions & 7 deletions js/src/forum/components/DiscussionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import DiscussionListItem from './DiscussionListItem';
import Button from '../../common/components/Button';
import LoadingIndicator from '../../common/components/LoadingIndicator';
import Placeholder from '../../common/components/Placeholder';
import Discussion from '../../common/models/Discussion';

/**
* The `DiscussionList` component displays a list of discussions.
Expand All @@ -13,37 +14,40 @@ import Placeholder from '../../common/components/Placeholder';
*/
export default class DiscussionList extends Component {
view() {
/**
* @type DiscussionListState
*/
const state = this.attrs.state;

const params = state.getParams();
let loading;

if (state.isLoading()) {
if (state.isInitialLoading() || state.isLoadingNext()) {
dsevillamartin marked this conversation as resolved.
Show resolved Hide resolved
loading = <LoadingIndicator />;
} else if (state.moreResults) {
} else if (state.hasNext()) {
loading = Button.component(
{
className: 'Button',
onclick: state.loadMore.bind(state),
onclick: state.loadNext.bind(state),
},
app.translator.trans('core.forum.discussion_list.load_more_button')
);
}

if (state.empty()) {
if (state.isEmpty()) {
const text = app.translator.trans('core.forum.discussion_list.empty_text');
return <div className="DiscussionList">{Placeholder.component({ text })}</div>;
}

return (
<div className={'DiscussionList' + (state.isSearchResults() ? ' DiscussionList--searchResults' : '')}>
<ul className="DiscussionList-discussions">
{state.discussions.map((discussion) => {
return (
{state.getPages().map((pg) => {
return pg.items.map((discussion) => (
<li key={discussion.id()} data-id={discussion.id()}>
{DiscussionListItem.component({ discussion, params })}
</li>
);
));
})}
</ul>
<div className="DiscussionList-loadMore">{loading}</div>
Expand Down
2 changes: 1 addition & 1 deletion js/src/forum/components/IndexPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default class IndexPage extends Page {
app.discussions.clear();
dsevillamartin marked this conversation as resolved.
Show resolved Hide resolved
}

app.discussions.refreshParams(app.search.params());
app.discussions.refreshParams(app.search.params(), m.route.param('page'));
dsevillamartin marked this conversation as resolved.
Show resolved Hide resolved

app.history.push('index', app.translator.trans('core.forum.header.back_to_index_tooltip'));

Expand Down
14 changes: 7 additions & 7 deletions js/src/forum/components/NotificationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Discussion from '../../common/models/Discussion';
export default class NotificationList extends Component {
view() {
const state = this.attrs.state;
const pages = state.getNotificationPages();
const pages = state.getPages();

return (
<div className="NotificationList">
Expand All @@ -30,12 +30,12 @@ export default class NotificationList extends Component {
</div>

<div className="NotificationList-content">
{pages.length
? pages.map((notifications) => {
{state.hasItems()
? pages.map((page) => {
const groups = [];
const discussions = {};

notifications.forEach((notification) => {
page.items.forEach((notification) => {
const subject = notification.subject();

if (typeof subject === 'undefined') return;
Expand Down Expand Up @@ -84,7 +84,7 @@ export default class NotificationList extends Component {
})
: ''}
{state.isLoading() ? (
<LoadingIndicator />
<LoadingIndicator className="LoadingIndicator--block" />
) : pages.length ? (
''
) : (
Expand Down Expand Up @@ -124,8 +124,8 @@ export default class NotificationList extends Component {
// by a fraction of a pixel, so we compensate for that.
const atBottom = Math.abs(scrollParent.scrollHeight - scrollParent.scrollTop - scrollParent.clientHeight) <= 1;

if (state.hasMoreResults() && !state.isLoading() && atBottom) {
state.loadMore();
if (state.hasNext() && !state.isLoadingNext() && atBottom) {
state.loadNext();
}
}

Expand Down
Loading