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

[SIEM] Changes authentications table from load more to paginated #39474

Merged
merged 37 commits into from
Jul 11, 2019

Conversation

stephmilovic
Copy link
Contributor

Summary

Convert Authentications Table pagination from "Load More" to numbered pagination.

How this works: Say we want to fetch results 90-100. We use the terms aggregation and query for 100 results. Then on the FE server, we splice the results and grab 90-100. Because of this implementation, the risk with numbered pagination is a that huge computation to get the last page. Therefore, I prevent the user from being able to go forward more than 5 pages initially, and after that only one page at a time. That math is done in /public/components/paginated_table/helpers.ts, under fakePossibleCount

Once this has passed code review, I plan on implementing this paging on all other tables. Speak now on implementation details or forever hold your peace!!

11

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@stephmilovic stephmilovic added loe:medium Medium Level of Effort Team:SIEM labels Jun 21, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/secops

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jun 26, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@MichaelMarcialis
Copy link
Contributor

Hey, @stephmilovic. From a design perspective, I have two suggested changes I wish to make:

  1. Currently the alignment of the pagination numbers and arrows appears to be somewhat center aligned. Ideally, I'd like to see these right aligned with the page contents. Assuming you're using EuiFlexGroup to layout the pagination, it should be as simple as setting the EuiFlexItem container for the pagination number to have a prop of grow={false}.

  2. The solution of only showing the first five pages of table data makes sense given the current technical limitations, but it may cause the user to inadvertently think that there are only five pages of data in total (when in fact there may be many more). I think a possible remedy to this would be to include the standard pagination ellipsis after the number 5 page, in situations where the number of pages actually exceeds five. Doing so will at the very least indicate to the user that there are more than five pages, even if they can't quickly navigate to them. For example:

image

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic requested a review from XavierM July 11, 2019 19:33
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM for the new pagination!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic merged commit bfee75e into elastic:master Jul 11, 2019
@stephmilovic stephmilovic deleted the paginated-table branch July 11, 2019 20:56
stephmilovic added a commit that referenced this pull request Jul 16, 2019
@FrankHassanabad FrankHassanabad changed the title [SIEM] - Authentications Table, Numbered Pagination [SIEM] Changes authentications table from load more to paginated Jul 30, 2019
@FrankHassanabad FrankHassanabad added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Jul 30, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants