-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[EuiInMemoryTable] Persist table rows per page and sort #198297
Conversation
/ci |
16 similar comments
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
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.
SharedUX changes LGTM. Reviewed the code and tests
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
cc @sebelga |
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.
ML changes LGTM
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.
kibana-presentation changes LGTM
code review only
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.
response-ops
LGTM 👍
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.
Changes LGTM, tested locally 👍
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.
LGTM, tested locally and works as expected. Great improvement, soon it will be hard to imagine, that users always had to manually restore there page size and sorting.
I see a potential to further improve this by aligning default page sizes. There are interesting selections we provide. Like:
const PAGE_SIZE_OPTIONS = [5, 10, 15, 25];
and default size 10
Here is potential align, centralize for a bit of a less fragmented UX.
But of course not in this PR, enjoy your long weekend!
...agement/public/components/edit_index_pattern/indexed_fields_table/components/table/table.tsx
Show resolved
Hide resolved
..._management/public/components/edit_index_pattern/relationships_table/relationships_table.tsx
Show resolved
Hide resolved
src/plugins/data/public/utils/table_inspector_view/components/data_table.tsx
Show resolved
Hide resolved
Thanks for the review @kertal !
I think that each domain will have their own specific so it's ok to have different page size based on the content presented. I agree that often |
Starting backport for target branches: 8.x https://github.com/elastic/kibana/actions/runs/11893584489 |
(cherry picked from commit 020acbe)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
… (#200569) # Backport This will backport the following commits from `main` to `8.x`: - [[EuiInMemoryTable] Persist table rows per page and sort (#198297)](#198297) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Sébastien Loix","email":"sebastien.loix@elastic.co"},"sourceCommit":{"committedDate":"2024-11-18T13:35:26Z","message":"[EuiInMemoryTable] Persist table rows per page and sort (#198297)","sha":"020acbeaa38c0807db4dc1d5ebdb7b112b484f1e","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Embedding","release_note:skip","v9.0.0","Team:SharedUX","backport:prev-minor"],"title":"[EuiInMemoryTable] Persist table rows per page and sort","number":198297,"url":"https://github.com/elastic/kibana/pull/198297","mergeCommit":{"message":"[EuiInMemoryTable] Persist table rows per page and sort (#198297)","sha":"020acbeaa38c0807db4dc1d5ebdb7b112b484f1e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/198297","number":198297,"mergeCommit":{"message":"[EuiInMemoryTable] Persist table rows per page and sort (#198297)","sha":"020acbeaa38c0807db4dc1d5ebdb7b112b484f1e"}}]}] BACKPORT--> Co-authored-by: Sébastien Loix <sebastien.loix@elastic.co>
In this PR I've added local storage persistence to the
EuiInMemoryTable
which saves the "rows per page" state of the tables. I decided to split the work into multiple PRs to make reviewing easier. In this PR I've done the tables of the "packages" and "src/plugins" folders.Partially fixes #56406
Note on implementation
In order to persist the data we use either the
useEuiTablePersist
hook or thewithEuiTablePersist
HOC from the@kbn/shared-ux-table-persist
package.Tables updated
Note to reviewers: check to checkbox once you have manually tested the table and it works as expected
<TableListView />
| @elastic/appex-shareduxThis table is used in the following components:
<AlertFieldsTable />
| @elastic/response-ops<QueryHistory />
| @elastic/kibana-esql (Only the sort field and direction is persisted.)<SearchSessionsMgmtTable />
| @elastic/kibana-data-discovery<DataTableFormat />
| @elastic/kibana-data-discovery<RelationshipsTable />
| @elastic/kibana-data-discovery<Table />
| @elastic/kibana-data-discovery<Table />
| @elastic/kibana-data-discovery<Table />
| @elastic/kibana-data-discovery<IndexPatternTable />
| @elastic/kibana-data-discovery<SavedObjectFinder />
| @elastic/appex-shareduxThis table is used in the following components: