Skip to content

[Stack Monitoring] fix useTable sorting and pagination#113563

Merged
neptunian merged 2 commits intoelastic:masterfrom
neptunian:fix-use-table
Oct 1, 2021
Merged

[Stack Monitoring] fix useTable sorting and pagination#113563
neptunian merged 2 commits intoelastic:masterfrom
neptunian:fix-use-table

Conversation

@neptunian
Copy link
Copy Markdown
Contributor

@neptunian neptunian commented Sep 30, 2021

  • Fixes sorting issue when saving to local storage
  • Fixes tables that requests data on table change to not make multiple requests by updating state again

Test

  • Sorting columns in a table should work
  • Sorting columns, refreshing the page, the column should remain sorted
  • Selecting page size should work. After refreshing the page it should remain.
  • Querying for a table item should work. If results are paginated make sure when paginating the query wasn't lost
  • In tables that make endpoint requests on change, only one request should be made when sorting or on pagination (try nodes list or pipeline listing)
  • Test all the above in tables that do not make requests (beats listing, indices listing)
  • Test all the above in the angular version as well. Switching between modes should work as local storage should not change.

@neptunian neptunian added v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.16.0 Epic: Stack Monitoring de-angularization labels Sep 30, 2021
@neptunian neptunian self-assigned this Sep 30, 2021
@neptunian neptunian requested a review from a team as a code owner September 30, 2021 18:10
@neptunian neptunian requested review from a team September 30, 2021 18:10
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)


// get initial state from localStorage
const [sorting, setSorting] = useState<Sorting>(storageData.sort || { sort: {} });
const cleanSortingData = (sortData: Sorting) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this new function was causing the sorting to store itself incorrectly. it doesn't exist in the angular version and after removal it stopped breaking

sorting,
pagination,
onTableChange,
fetchMoreData: ({
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This used to have the async updateData function in here (https://github.com/elastic/kibana/blob/master/x-pack/plugins/monitoring/public/views/base_eui_table_controller.js#L131). Now it does not and this already happens onTableChange

}}
onTableChange={onTableChange}
fetchMoreData={fetchMoreData}
{...props}
Copy link
Copy Markdown
Contributor Author

@neptunian neptunian Sep 30, 2021

Choose a reason for hiding this comment

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

don't look for fetchMoreData explicitly because its removed in the react version now

sorting={sortingOptions}
message={upgradeMessage}
pagination={pagination}
fetchMoreData={fetchMoreData}
Copy link
Copy Markdown
Contributor Author

@neptunian neptunian Sep 30, 2021

Choose a reason for hiding this comment

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

don't look for fetchMoreData explicitly because its removed in the react version now

}
// react version
else {
onTableChange({ page, sort, queryText });
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fetchMoreData isn't needed in React version because we get data in the ElasticsearchTemplate

items={items}
pagination={pagination}
onChange={onChange}
loading={isLoading}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No loading visual was happening in Angular version so I removed it. Would need to pass this down from parent component at some point.

@neptunian
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@matschaffer
Copy link
Copy Markdown
Contributor

matschaffer commented Oct 1, 2021

testing

using monitoring-olbt, edge-olbt didn't seem to have any monitoring data

  • Sorting columns in a table should work
  • Sorting columns, refreshing the page, the column short remain sorted
  • Selecting page size should work. After refreshing the page it should remain.
  • Querying for a table item should work. If results are paginated make sure when paginating the query wasn't lost
  • In tables that make endpoint requests on change, only one request should be made when sorting or on pagination (tested on node list)
  • Test all the above in tables that do not make requests (tested on index list)
  • Test all the above in the angular version as well. Switching between modes should work as local storage should not change.

@matschaffer
Copy link
Copy Markdown
Contributor

matschaffer commented Oct 1, 2021

I noticed that the filter got lost after refresh, but sorting remained. Angular master and even 7.15.0 do this so seems intended. Just mentioning incase anyone flags it as a bug.

Copy link
Copy Markdown
Contributor

@matschaffer matschaffer left a comment

Choose a reason for hiding this comment

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

Looks solid. Thanks for posting the recommended test steps!

@neptunian neptunian added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 1, 2021
@neptunian neptunian merged commit 56effce into elastic:master Oct 1, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 1, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 1, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Sandra G <neptunian@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Oct 6, 2021

⏳ Build in-progress, with failures

Failed CI Steps

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @neptunian

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

Labels

auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants