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.

Sign up for GitHub

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

Jump to bottom

Enable org queues to use task pagination API #11213

Merged
merged 23 commits into from Jun 27, 2019
Merged

Enable org queues to use task pagination API #11213

Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
e57d639
Enable org queues to use task pagination API
lowellrex Jun 25, 2019
9849fdd
Actually requesting pages from the API now
lowellrex Jun 25, 2019
3a050cb
Add key to prevent sharing state across sibling components
lowellrex Jun 25, 2019
65769d3
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 25, 2019
e36abdb
Rename task collections for clarity
lowellrex Jun 25, 2019
420d027
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 25, 2019
e5cba68
Update TablePagination test
lowellrex Jun 25, 2019
6aca957
Satisfy eslint
lowellrex Jun 25, 2019
ef6c091
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 26, 2019
219cb22
Update test because shape of config changed
lowellrex Jun 26, 2019
3ea371d
Use built-in total_pages from kaminari
lowellrex Jun 26, 2019
0650e67
Replace TablePagination with Pagination
lowellrex Jun 26, 2019
6d268dc
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 26, 2019
322402e
Fix sloppy merge
lowellrex Jun 26, 2019
25090e4
Use QueueTable directly in OrganizationQueue
lowellrex Jun 26, 2019
99c52ba
Implement component-level caching
lowellrex Jun 26, 2019
f631fa5
Increase test coverage
lowellrex Jun 26, 2019
8d0172b
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 27, 2019
38722a7
Remove caching
lowellrex Jun 27, 2019
eccf405
Remove manual testing toggle
lowellrex Jun 27, 2019
217f45e
Merge branch 'master' into lowell/11054_frontend_pagination_table
lowellrex Jun 27, 2019
6d724d9
Merge branch 'master' into lowell/11054_frontend_pagination_table
Jun 27, 2019
7e872f9
Merge branch 'master' into lowell/11054_frontend_pagination_table
Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 20 additions & 4 deletions client/app/queue/QueueTable.jsx
View file
Open in desktop
Expand Up @@ -195,7 +195,7 @@ export default class QueueTable extends React.PureComponent {
sortColIdx: null,
areDropdownFiltersOpen: {},
filteredByList: {},
taskPageFromApi: [],
tasksFromApiForPage: {},
loadingComponent: null,
currentPage: 0
};
Expand Down Expand Up @@ -286,6 +286,15 @@ export default class QueueTable extends React.PureComponent {
return paginatedData;
}

componentDidMount = () => {
this.setState({
tasksFromApiForPage: {
Copy link
Contributor

Choose a reason for hiding this comment

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

as I'm reading this, the idea seems to be cache each paged task set on the browser, rather than re-fetching on each page click?

I can see the attraction of that for performance reasons. My only concern would be once sorting/filtering are enabled, that the cache invalidation will be tricky. Also, if someone updates a task in a different browser, that the user in this browser won't see the latest when they page "back" to that task set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I think we'll have to handle caching some other way when we do sorting/filtering.

And you're totally right about the different browser window/tab causing cached tasks to be in the incorrect state (though we already have this now with entirely front-end pagination).

Happy to back out the caching of paged tasks if you think that's better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the caching falls under the category of premature optimization. Let's back it out. In theory the pagination itself should make things perform well enough that we don't need caching -- time will tell.

...this.state.tasksFromApiForPage,
...{ [this.state.currentPage]: this.props.rowObjects }
}
});
}

updateCurrentPage = (newPage) => {
this.setState({ currentPage: newPage });
this.requestNewPage(newPage);
Expand All @@ -296,6 +305,10 @@ export default class QueueTable extends React.PureComponent {
return;
}

if (this.state.tasksFromApiForPage[newPage]) {
return;
}

this.setState({ loadingComponent: <LoadingScreen spinnerColor={LOGO_COLORS.QUEUE.ACCENT} /> });

// Request newPage + 1 since our API indexes starting at 1 and the pagination element indexes starting at 0.
Expand All @@ -305,7 +318,10 @@ export default class QueueTable extends React.PureComponent {
} = JSON.parse(response.text);

this.setState({
taskPageFromApi: tasksWithAppealsFromRawTasks(tasks),
tasksFromApiForPage: {
...this.state.tasksFromApiForPage,
...{ [newPage]: tasksWithAppealsFromRawTasks(tasks) }
},
loadingComponent: null
});
}).
Expand Down Expand Up @@ -341,8 +357,8 @@ export default class QueueTable extends React.PureComponent {
} = this.props;

if (useTaskPagesApi) {
if (this.state.taskPageFromApi.length) {
rowObjects = this.state.taskPageFromApi;
if (this.state.tasksFromApiForPage[this.state.currentPage]) {
rowObjects = this.state.tasksFromApiForPage[this.state.currentPage];
}
} else {
// Steps to calculate table data to display:
Expand Down