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

[EuiInMemoryTable] Pagination breaks when search.onChange is configured #3445

Closed
cjcenizal opened this issue May 8, 2020 · 3 comments
Closed
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries stale-issue stale-issue-closed

Comments

@cjcenizal
Copy link
Contributor

To reproduce this problem, configure an EuiInMemoryTable with something like this:

const search = {
  onChange: (data) => this.setState(queryText: data.query.text),
  box: {
    incremental: true,
  },
};

// The items fed to the table will typically use the query text as a filter.
// This is a React best practice.
const filteredItems = this.state.items.filter(item => item.name.includes(this.state.queryText));

The bug arises due to the new array being generated each render by the filtering logic. EuiInMemoryTable deliberately treats a new array as a trigger for resetting pagination to the first page. This manifests as nonresponsive pagination whenever search.onChange is configured.

After discussing this with @chandlerprall we concluded that a reasonable solution would be to document best practices on the example page, which is to cache the filtered rows on state instead of calculating them inside of the render method. Documentation would include things like:

  • Updating an example to configure search.onChange and demonstrate this best practice.
  • Adding a comment to the example code pointing out the reason for caching the filtered result on state.
  • Adding a callout to describe this best practice and reason for it, since it contradicts conventional React best practices.
@cjcenizal cjcenizal added documentation Issues or PRs that only affect documentation - will not need changelog entries tables labels May 8, 2020
@cjcenizal
Copy link
Contributor Author

As you can see in elastic/kibana#65931, the solution can get a little more complicated when you consider that items might be deleted (or added). In these cases, table won't update, because the original list of items no longer acts as an input to the render method. This requires the introduction of a getDerivedStateFromProps method to update the filtered items cache when the list of items changes.

@cchaos cchaos changed the title EuiInMemoryTable pagination breaks when search.onChange is configured [EuiInMemoryTable] Pagination breaks when search.onChange is configured Sep 19, 2020
@cchaos cchaos removed the tables label Sep 19, 2020
@github-actions
Copy link

👋 Hey there. This issue hasn't had any activity for 180 days. We'll automatically close it if that trend continues for another week. If you feel this issue is still valid and needs attention please let us know with a comment.

@github-actions
Copy link

❌ We're automatically closing this issue due to lack of activity. Please comment if you feel this was done in error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries stale-issue stale-issue-closed
Projects
None yet
Development

No branches or pull requests

2 participants