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

Pagination refactoring/cleanup #3205

Merged
merged 19 commits into from
Oct 30, 2023
Merged

Pagination refactoring/cleanup #3205

merged 19 commits into from
Oct 30, 2023

Conversation

teodosii
Copy link
Member

@teodosii teodosii commented Oct 27, 2023

What this PR does

  • Pagination cleanup so that frontend no longer hardcodes page size
  • Moved pagination to the filters store instead for all pages that have pagination enabled
  • Prevent UI triggering polling if window is inactive or if the previous request didn't complete

Which issue(s) this PR fixes

#2931
#2462

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • CHANGELOG.md updated (or pr:no changelog PR label added if not required)

@teodosii teodosii requested a review from a team October 27, 2023 08:20
@teodosii teodosii added the pr:no public docs Added to a PR that does not require public documentation updates label Oct 27, 2023
@@ -78,27 +76,29 @@ class Incidents extends React.Component<IncidentsPageProps, IncidentsPageState>

const cursor = cursorQuery || undefined;
const start = !isNaN(startQuery) ? Number(startQuery) : 1;
const itemsPerPage = !isNaN(perpageQuery) ? Number(perpageQuery) : ITEMS_PER_PAGE;

const end = !isNaN(perpageQuery) ? Number(perpageQuery) : undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

This will set it to either the query's values or to 1 and undefined, where the end will be determined when fetching the results

componentDidMount() {
const { alertGroupStore } = this.props.store;

alertGroupStore.updateBulkActions();
Copy link
Member Author

@teodosii teodosii Oct 27, 2023

Choose a reason for hiding this comment

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

These belong in the cDM and not in the constructor.

Copy link
Contributor

@brojd brojd left a comment

Choose a reason for hiding this comment

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

Few comments / questions about the code itself, I'm not ready to assess any aspect of the business logic though:)

@brojd brojd self-requested a review October 27, 2023 12:36
<CursorPagination
current={`${pagination.start}-${pagination.end}`}
itemsPerPage={alertGroupStore.incidentsItemsPerPage}
itemsPerPageOptions={[
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally those options should depend on page_size

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm unfortunately the paging on alert group is slightly different than what we have traditionally in the other places. You kind of need the ends of the pagination 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh nvm, that can be changed. I thought you were referring to the pagination state

@@ -294,21 +280,30 @@ export class AlertGroupStore extends BaseStore {
// @ts-ignore
this.alerts = new Map<number, Alert>([...this.alerts, ...newAlerts]);

this.incidentsItemsPerPage = page_size;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need to store this in alertgroupstore as a separate field? can we get it as getSearchResult().page_size?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess you're right, let me try it 🤔

Copy link
Member Author

@teodosii teodosii Oct 30, 2023

Choose a reason for hiding this comment

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

This just doesn't seem to work quite as expected if we only replace it with getSearchResult() because there's no way to trigger the pagination update nested in the state of the component 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

weird but ok

@teodosii teodosii added this pull request to the merge queue Oct 30, 2023
Merged via the queue into dev with commit 871860c Oct 30, 2023
20 of 21 checks passed
@teodosii teodosii deleted the rares/filters-pagination branch October 30, 2023 14:25
brojd pushed a commit that referenced this pull request Sep 18, 2024
# What this PR does

- Pagination cleanup so that frontend no longer hardcodes page size
- Moved pagination to the filters store instead for all pages that have
pagination enabled
- Prevent UI triggering polling if window is inactive or if the previous
request didn't complete

## Which issue(s) this PR fixes

#2931 
#2462

## Checklist

- [ ] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants